[PATCH] objtool: prefer memory clobber & %= to volatile & __COUNTER__

From: Nick Desaulniers
Date: Thu Jan 13 2022 - 20:05:43 EST


commit dcce50e6cc4d ("compiler.h: Fix annotation macro misplacement with Clang")
mentions:

> 'volatile' is ignored for some reason and Clang feels free to move the
> reachable() annotation away from its intended location.

Indeed, volatile is not a compiler barrier. Particularly once `-march=`
flags are used under certain configs, LLVM's machine-scheduler can be
observed moving instructions across the asm statement meant to point to
known reachable or unreachable code, as reported by 0day bot.

Prefer a memory clobber which is a compiler barrier that prevents these
re-orderings and remove the volatile qualifier.

Looking closer, the use of __COUNTER__ seems to have been used to
prevent de-duplication of these asm statements. The GCC manual mentions:

> Under certain circumstances, GCC may duplicate (or remove duplicates
> of) your assembly code when optimizing. This can lead to unexpected
> duplicate symbol errors during compilation if your asm code defines
> symbols or labels. Using ‘%=’ (see AssemblerTemplate) may help resolve
> this problem.
>
> ‘%=’ Outputs a number that is unique to each instance of the asm
> statement in the entire compilation. This option is useful when
> creating local labels and referring to them multiple times in a single
> template that generates multiple assembler instructions.

commit 3d1e236022cc ("objtool: Prevent GCC from merging annotate_unreachable()")

Mentions that

> The inline asm ‘%=’ token could be used for that, but unfortunately
> older versions of GCC don't support it.

>From testing all versions of GCC available on godbolt.org, GCC 4.1+
seems to support 4.1. Since the minimum supported version of GCC at the
moment is GCC 5.1, it sounds like this is no longer a concern.

Prefer the %= assembler template to having to stringify __COUNTER__.

This commit is effectively a revert of the following commits:
commit dcce50e6cc4d ("compiler.h: Fix annotation macro misplacement with Clang")
commit f1069a8756b9 ("compiler.h: Avoid using inline asm operand modifiers")
commit c199f64ff93c ("instrumentation.h: Avoid using inline asm operand modifiers")
commit d0c2e691d1cb ("objtool: Add a comment for the unreachable annotation macros")
commit ec1e1b610917 ("objtool: Prevent GCC from merging annotate_unreachable(), take 2")
commit 3d1e236022cc ("objtool: Prevent GCC from merging annotate_unreachable()")

Link: https://github.com/ClangBuiltLinux/linux/issues/1566
Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile
Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#AssemblerTemplate
Link: https://lore.kernel.org/llvm/202112080834.XFYU8b5Q-lkp@xxxxxxxxx/
Link: https://lore.kernel.org/llvm/202111300857.IyINAyJk-lkp@xxxxxxxxx/
Reported-by: kernel test robot <lkp@xxxxxxxxx>
Debugged-by: Nathan Chancellor <nathan@xxxxxxxxxx>
Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
---
include/linux/compiler.h | 31 +++++++++++--------------------
include/linux/instrumentation.h | 24 ++++++++++--------------
2 files changed, 21 insertions(+), 34 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 429dcebe2b99..3ac21b888d20 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -108,30 +108,21 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
# define barrier_before_unreachable() do { } while (0)
#endif

-/* Unreachable code */
+/* These macros help objtool understand GCC code flow for unreachable code. */
#ifdef CONFIG_STACK_VALIDATION
-/*
- * These macros help objtool understand GCC code flow for unreachable code.
- * The __COUNTER__ based labels are a hack to make each instance of the macros
- * unique, to convince GCC not to merge duplicate inline asm statements.
- */
-#define __stringify_label(n) #n
-
-#define __annotate_reachable(c) ({ \
- asm volatile(__stringify_label(c) ":\n\t" \
- ".pushsection .discard.reachable\n\t" \
- ".long " __stringify_label(c) "b - .\n\t" \
- ".popsection\n\t" : : "i" (c)); \
+#define annotate_reachable() ({ \
+ asm (".Lreachable%=:\n\t" \
+ ".pushsection .discard.reachable\n\t" \
+ ".long .Lreachable%= - .\n\t" \
+ ".popsection\n\t" ::: "memory"); \
})
-#define annotate_reachable() __annotate_reachable(__COUNTER__)

-#define __annotate_unreachable(c) ({ \
- asm volatile(__stringify_label(c) ":\n\t" \
- ".pushsection .discard.unreachable\n\t" \
- ".long " __stringify_label(c) "b - .\n\t" \
- ".popsection\n\t" : : "i" (c)); \
+#define annotate_unreachable() ({ \
+ asm (".Lunreachable%=:\n\t" \
+ ".pushsection .discard.unreachable\n\t" \
+ ".long .Lunreachable%= - .\n\t" \
+ ".popsection\n\t" ::: "memory"); \
})
-#define annotate_unreachable() __annotate_unreachable(__COUNTER__)

#define ASM_UNREACHABLE \
"999:\n\t" \
diff --git a/include/linux/instrumentation.h b/include/linux/instrumentation.h
index 24359b4a9605..0dae9c08764f 100644
--- a/include/linux/instrumentation.h
+++ b/include/linux/instrumentation.h
@@ -4,16 +4,13 @@

#if defined(CONFIG_DEBUG_ENTRY) && defined(CONFIG_STACK_VALIDATION)

-#include <linux/stringify.h>
-
/* Begin/end of an instrumentation safe region */
-#define __instrumentation_begin(c) ({ \
- asm volatile(__stringify(c) ": nop\n\t" \
- ".pushsection .discard.instr_begin\n\t" \
- ".long " __stringify(c) "b - .\n\t" \
- ".popsection\n\t" : : "i" (c)); \
+#define instrumentation_begin() ({ \
+ asm (".Linstbegin%=: nop\n\t" \
+ ".pushsection .discard.instr_begin\n\t" \
+ ".long .Linstbegin%= - .\n\t" \
+ ".popsection\n\t" ::: "memory"); \
})
-#define instrumentation_begin() __instrumentation_begin(__COUNTER__)

/*
* Because instrumentation_{begin,end}() can nest, objtool validation considers
@@ -46,13 +43,12 @@
* To avoid this, have _end() be a NOP instruction, this ensures it will be
* part of the condition block and does not escape.
*/
-#define __instrumentation_end(c) ({ \
- asm volatile(__stringify(c) ": nop\n\t" \
- ".pushsection .discard.instr_end\n\t" \
- ".long " __stringify(c) "b - .\n\t" \
- ".popsection\n\t" : : "i" (c)); \
+#define instrumentation_end() ({ \
+ asm (".Linstend%=: nop\n\t" \
+ ".pushsection .discard.instr_end\n\t" \
+ ".long .Linstend%= - .\n\t" \
+ ".popsection\n\t" ::: "memory"); \
})
-#define instrumentation_end() __instrumentation_end(__COUNTER__)
#else
# define instrumentation_begin() do { } while(0)
# define instrumentation_end() do { } while(0)

base-commit: dcce50e6cc4d86a63dc0a9a6ee7d4f948ccd53a1
--
2.34.1.703.g22d0c6ccf7-goog