| Message ID | 20220708231520.3958391-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] coresight: etm4x: avoid build failure with unrolled loops | expand |
On Sat, Jul 9, 2022 at 1:15 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > ({ \ > u64 __val; \ > \ > - if (__builtin_constant_p((offset))) \ > + if (__is_constexpr((offset))) \ > __val = read_etm4x_sysreg_const_offset((offset)); \ > else \ > __val = etm4x_sysreg_read((offset), true, (_64bit)); \ > This is clearly better than the current version using __builtin_constant_p(), but I don't think it's safe in all cases, since there are expressions that are constant expressions to the compiler but are not valid input to the assembler. I would prefer to see this fixed differently, but doing this one first is also fine with me: Acked-by: Arnd Bergmann <arnd@arndb.de> Arnd
On 10/07/2022 20:13, Arnd Bergmann wrote: > On Sat, Jul 9, 2022 at 1:15 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > >> ({ \ >> u64 __val; \ >> \ >> - if (__builtin_constant_p((offset))) \ >> + if (__is_constexpr((offset))) \ >> __val = read_etm4x_sysreg_const_offset((offset)); \ >> else \ >> __val = etm4x_sysreg_read((offset), true, (_64bit)); \ >> > > This is clearly better than the current version using > __builtin_constant_p(), but > I don't think it's safe in all cases, since there are expressions that > are constant > expressions to the compiler but are not valid input to the assembler. > > I would prefer to see this fixed differently, but doing this one first is also > fine with me: > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > Arnd Thanks, I have queued this. Suzuki
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h index 33869c1d20c3..a7bfea31f7d8 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.h +++ b/drivers/hwtracing/coresight/coresight-etm4x.h @@ -7,6 +7,7 @@ #define _CORESIGHT_CORESIGHT_ETM_H #include <asm/local.h> +#include <linux/const.h> #include <linux/spinlock.h> #include <linux/types.h> #include "coresight-priv.h" @@ -515,7 +516,7 @@ ({ \ u64 __val; \ \ - if (__builtin_constant_p((offset))) \ + if (__is_constexpr((offset))) \ __val = read_etm4x_sysreg_const_offset((offset)); \ else \ __val = etm4x_sysreg_read((offset), true, (_64bit)); \
When the following configs are enabled: * CORESIGHT * CORESIGHT_SOURCE_ETM4X * UBSAN * UBSAN_TRAP Clang fails assemble the kernel with the error: <instantiation>:1:7: error: expected constant expression in '.inst' directive .inst (0xd5200000|((((2) << 19) | ((1) << 16) | (((((((((((0x160 + (i * 4))))) >> 2))) >> 7) & 0x7)) << 12) | ((((((((((0x160 + (i * 4))))) >> 2))) & 0xf)) << 8) | (((((((((((0x160 + (i * 4))))) >> 2))) >> 4) & 0x7)) << 5)))|(.L__reg_num_x8)) ^ drivers/hwtracing/coresight/coresight-etm4x-core.c:702:4: note: while in macro instantiation etm4x_relaxed_read32(csa, TRCCNTVRn(i)); ^ drivers/hwtracing/coresight/coresight-etm4x.h:403:4: note: expanded from macro 'etm4x_relaxed_read32' read_etm4x_sysreg_offset((offset), false))) ^ drivers/hwtracing/coresight/coresight-etm4x.h:383:12: note: expanded from macro 'read_etm4x_sysreg_offset' __val = read_etm4x_sysreg_const_offset((offset)); \ ^ drivers/hwtracing/coresight/coresight-etm4x.h:149:2: note: expanded from macro 'read_etm4x_sysreg_const_offset' READ_ETM4x_REG(ETM4x_OFFSET_TO_REG(offset)) ^ drivers/hwtracing/coresight/coresight-etm4x.h:144:2: note: expanded from macro 'READ_ETM4x_REG' read_sysreg_s(ETM4x_REG_NUM_TO_SYSREG((reg))) ^ arch/arm64/include/asm/sysreg.h:1108:15: note: expanded from macro 'read_sysreg_s' asm volatile(__mrs_s("%0", r) : "=r" (__val)); \ ^ arch/arm64/include/asm/sysreg.h:1074:2: note: expanded from macro '__mrs_s' " mrs_s " v ", " __stringify(r) "\n" \ ^ Consider the definitions of TRCSSCSRn and TRCCNTVRn: drivers/hwtracing/coresight/coresight-etm4x.h:56 #define TRCCNTVRn(n) (0x160 + (n * 4)) drivers/hwtracing/coresight/coresight-etm4x.h:81 #define TRCSSCSRn(n) (0x2A0 + (n * 4)) Where the macro parameter is expanded to i; a loop induction variable from etm4_disable_hw. When any compiler can determine that loops may be unrolled, then the __builtin_constant_p check in read_etm4x_sysreg_offset() defined in drivers/hwtracing/coresight/coresight-etm4x.h may evaluate to true. This can lead to the expression `(0x160 + (i * 4))` being passed to read_etm4x_sysreg_const_offset. Via the trace above, this is passed through READ_ETM4x_REG, read_sysreg_s, and finally to __mrs_s where it is string-ified and used directly in inline asm. Regardless of which compiler or compiler options determine whether a loop can or can't be unrolled, which determines whether __builtin_constant_p evaluates to true when passed an expression using a loop induction variable, it is NEVER safe to allow the preprocessor to construct inline asm like: asm volatile (".inst (0x160 + (i * 4))" : "=r"(__val)); ^ expected constant expression Instead of read_etm4x_sysreg_offset() using __builtin_constant_p(), use __is_constexpr from include/linux/const.h instead to ensure only expressions that are valid integer constant expressions get passed through to read_sysreg_s(). This is not a bug in clang; it's a potentially unsafe use of the macro arguments in read_etm4x_sysreg_offset dependent on __builtin_constant_p. Link: https://github.com/ClangBuiltLinux/linux/issues/1310 Reported-by: Arnd Bergmann <arnd@kernel.org> Reported-by: Tao Zhang <quic_taozha@quicinc.com> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes v5 -> v6: * Rework patch to replace use of __builtin_constant_p with __is_constexpr in read_etm4x_sysreg_offset(). * Drop Suzuki's suggested by tag, since this is a different approach. * Convert Tao's suggested by tag to a reported by tag. * Reword bottom part of commit message. V5 (Nick): https://lore.kernel.org/llvm/20220623174131.3818333-1-ndesaulniers@google.com/ V4 (Nick): https://lore.kernel.org/llvm/20220614220229.1640085-1-ndesaulniers@google.com/ V3 (Tao): https://lore.kernel.org/lkml/1632652550-26048-1-git-send-email-quic_taozha@quicinc.com/ V2 (Arnd): https://lore.kernel.org/lkml/20210429145752.3218324-1-arnd@kernel.org/ V1 (Arnd): https://lore.kernel.org/lkml/20210225094324.3542511-1-arnd@kernel.org/ drivers/hwtracing/coresight/coresight-etm4x.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) base-commit: 525496a030de4ae64bb9e1d6bfc88eec6f5fe6e2