coresight: etm4x: work around clang-12+ build failure
PATCH 1/1 coresight: etm4x: work around clang-12+ build failure
From: Arnd Bergmann <[email protected]>
clang-12 fails to build the etm4x driver with -fsanitize=array-bounds:
<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" \
^
It appears that the __builin_constant_p() check in read_etm4x_sysreg_offset() falsely returns 'true' here because clang decides finds that an out-of-bounds access to config->cntr_val[] cannot happen, and then it unrolls the loop with constant register numbers. Then when actually emitting the output, it fails to figure out the value again.
While this is incorrect behavior in clang, it is easy to work around by avoiding the out-of-bounds array access. Do this by limiting the loop counter to the actual dimension of the array.
--- drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/hwtracing/coresight/coresight-etm4x-core.c +2 -2
@@ -691,13 +691,13 @@ static void etm4_disable_hw(void *info) "timeout while waiting for PM stable Trace Status\n");/* read the status of the single shot comparators */ -for (i =0 ; i < drvdata->nr_ss_cmp; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_ss_cmp,ETM_MAX_SS_CMP ); i++) { config->ss_status[i] =etm4x_relaxed_read32 (csa,TRCSSCSRn (i)); }/* read back the current counter values */ -for (i =0 ; i < drvdata->nr_cntr; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_cntr, ETMv4_MAX_CNTR); i++) { config->cntr_val[i] =etm4x_relaxed_read32 (csa,TRCCNTVRn (i)); }
Signature · 1 line
2.29.2
Good morning,
Quoted 52 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
clang-12 fails to build the etm4x driver with -fsanitize=array-bounds:
<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" \
^It appears that the
__builin_constant_p()check inread_etm4x_sysreg_offset()falsely returns 'true' here because clang decides finds that an out-of-bounds access to config->cntr_val[] cannot happen, and then it unrolls the loop with constant register numbers. Then when actually emitting the output, it fails to figure out the value again.While this is incorrect behavior in clang, it is easy to work around by avoiding the out-of-bounds array access. Do this by limiting the loop counter to the actual dimension of the array.
Signed-off-by Arnd Bergmann <[email protected]>--- drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/hwtracing/coresight/coresight-etm4x-core.c +0
-0@@ -691,13 +691,13 @@ static void etm4_disable_hw(void *info) "timeout while waiting for PM stable Trace Status\n");
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_disable_hw(void *info) · line 700
Quoted 10 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +2
-2/* read the status of the single shot comparators */ -for (i =0 ; i < drvdata->nr_ss_cmp; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_ss_cmp,ETM_MAX_SS_CMP ); i++) { config->ss_status[i] =etm4x_relaxed_read32 (csa,TRCSSCSRn (i)); }/* read back the current counter values */ -for (i =0 ; i < drvdata->nr_cntr; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_cntr, ETMv4_MAX_CNTR); i++) {
This patch will work and I'd be happy to apply it if this was the only instance, but there are dozens of places in the coresight drivers where such patterns are used. Why are those not flagged as well? And shouldn't the real fix be with clang?
Thanks,
Mathieu
Quoted 6 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +0
-0config->cntr_val[i] =etm4x_relaxed_read32 (csa,TRCCNTVRn (i)); }Signature · 1 line
2.29.2
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 14 lines · Mathieu Poirier
Quoted 13 lines · Arnd Bergmann
index
15016f757828..4cccf874a602100644drivers/hwtracing/coresight/coresight-etm4x-core.c +1
-1@@ -691,13 +691,13 @@ static void etm4_disable_hw(void *info) "timeout while waiting for PM stable Trace Status\n");/* read the status of the single shot comparators */ -for (i =0 ; i < drvdata->nr_ss_cmp; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_ss_cmp,ETM_MAX_SS_CMP ); i++) { config->ss_status[i] =etm4x_relaxed_read32 (csa,TRCSSCSRn (i)); }
Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_disable_hw(void *info) · line 700
Quoted 3 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +1
-1/* read back the current counter values */ -for (i =0 ; i < drvdata->nr_cntr; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_cntr, ETMv4_MAX_CNTR); i++) {This patch will work and I'd be happy to apply it if this was the only instance, but there are dozens of places in the coresight drivers where such patterns are used. Why are those not flagged as well? And shouldn't the real fix be with clang?
This is the only one I see in randconfig build tests. In fact I only actually see the second one of the two causing problems, but it seemed silly to change only one of them. I had not noticed the other similar loops that use etm4x_read32() instead of etm4x_relaxed_read32().
I first suspected that the memory clobber in __iormb() prevents the compiler from running into the same problem there, but changing all etm4x_read32() to etm4x_relaxed_read32() did not make it show up in other places either.
While I expect this will be fixed in mainline clang, the workaround should be harmless and let us use the existing releases as well.
Arnd
Quoted 37 lines · Mathieu Poirier
Good morning,
Quoted 33 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
clang-12 fails to build the etm4x driver with -fsanitize=array-bounds:
<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" \
^It appears that the
__builin_constant_p()check inread_etm4x_sysreg_offset()falsely returns 'true' here because clang decides finds that an out-of-bounds access to config->cntr_val[] cannot happen, and then it unrolls the loop with constant register numbers. Then
Is a sanitizer enabled, that would trap on OOB?
Quoted 26 lines
Quoted 26 lines
when actually emitting the output, it fails to figure out the value again.
While this is incorrect behavior in clang, it is easy to work around by avoiding the out-of-bounds array access. Do this by limiting the loop counter to the actual dimension of the array.
Signed-off-by Arnd Bergmann <[email protected]>--- drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/hwtracing/coresight/coresight-etm4x-core.c +1
-1@@ -691,13 +691,13 @@ static void etm4_disable_hw(void *info) "timeout while waiting for PM stable Trace Status\n");/* read the status of the single shot comparators */ -for (i =0 ; i < drvdata->nr_ss_cmp; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_ss_cmp,ETM_MAX_SS_CMP ); i++) { config->ss_status[i] =etm4x_relaxed_read32 (csa,TRCSSCSRn (i)); }
Quoted 8 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_disable_hw(void *info) · line 700
Quoted 3 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +1
-1/* read back the current counter values */ -for (i =0 ; i < drvdata->nr_cntr; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_cntr, ETMv4_MAX_CNTR); i++) {This patch will work and I'd be happy to apply it if this was the only instance, but there are dozens of places in the coresight drivers where such patterns are used. Why are those not flagged as well? And shouldn't the real fix be with clang?
It's important to understand the __builtin_constant_p is highly sensitive to optimizations; code using it typically relies on optimizations being performed before it's evaluated. Which optimizations, applied successfully or not, in what order, by which compiler or versions of the same compiler can affect what __builtin_constant_p evaluates to. Code generally needs to be written to assume that failure for __builtin_constant_p to evaluate to a specific value or not is _not a bug_.
Quoted 10 lines
Thanks,
MathieuQuoted 6 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +0
-1config->cntr_val[i] =etm4x_relaxed_read32 (csa,TRCCNTVRn (i)); } --2.29.2
Signature · 2 lines
Thanks, ~Nick Desaulniers
Quoted 4 lines · Nick Desaulniers' via Clang Built Linux
Quoted 1 line · Mathieu Poirier
Quoted 11 lines
Quoted 6 lines
Good morning,
Quoted 3 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
clang-12 fails to build the etm4x driver with -fsanitize=array-bounds:
Is a sanitizer enabled, that would trap on OOB?
Quoted 2 lines
Quoted 2 lines
As described over there, this happens only with the array-bounds sanitizer.
Actually, looking at it again now, in the reduced test case, the inline assembly is not even parsable, it only works because it never gets emitted without -fsanitize=array-bounds, and the alternative code path is used in
#define read_etm4x_sysreg_offset(offset, _64bit)
\
({
\
u64 __val;
\
\
if (__builtin_constant_p((offset)))
\
__val =
read_etm4x_sysreg_const_offset((offset)); \
else
\
__val = etm4x_sysreg_read((offset), true,
(_64bit)); \
__val;
\
})read_etm4x_sysreg_const_offset() eventually turns into something like
asm("msr_s " __stringify(offset));
so the offset has to be something that can be parsed by the assembler. __builtin_constant_p() checks that it is a constant value at compile-time, and in this case it can be because there is a small upper bound and clang just unrolls the loop.
I don't think there is an alternative to __builtin_constant_p() that can be used to decide if the argument is something that can be used as a constant expression in an inline assembly.
Arnd
Suzuki K Poulose
maintainerQuoted 25 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
clang-12 fails to build the etm4x driver with -fsanitize=array-bounds:
<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)); \ ^
Quoted 7 lines
arch/arm64/include/asm/sysreg.h:1074:2: note: expanded from macro '__mrs_s'
"mrs_s" v ", "__stringify(r)"\n" \
^It appears that the
__builin_constant_p()check inread_etm4x_sysreg_offset()falsely returns 'true' here because clang decides finds that an out-of-bounds access to config->cntr_val[] cannot
s/decides finds/decides/ ?
Quoted 18 lines
happen, and then it unrolls the loop with constant register numbers. Then when actually emitting the output, it fails to figure out the value again.
While this is incorrect behavior in clang, it is easy to work around by avoiding the out-of-bounds array access. Do this by limiting the loop counter to the actual dimension of the array.
Signed-off-by Arnd Bergmann <[email protected]>--- drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/hwtracing/coresight/coresight-etm4x-core.c +0
-0
drivers/hwtracing/coresight/coresight-etm4x-core.c static void etm4_disable_hw(void *info) · line 698
Quoted 9 lines
drivers/hwtracing/coresight/coresight-etm4x-core.c +1
-1@@ -691,13 +691,13 @@ static void etm4_disable_hw(void *info) "timeout while waiting for PM stable Trace Status\n");/* read the status of the single shot comparators */ -for (i =0 ; i < drvdata->nr_ss_cmp; i++) { +for (i =0 ; i <min_t (u32, drvdata->nr_ss_cmp,ETM_MAX_SS_CMP ); i++) { config->ss_status[i] =etm4x_relaxed_read32 (csa,TRCSSCSRn (i)); }
There are more places where we do this. So I believe you need the similar change elsewhere too in the driver. So, that becomes cumbersome for circumventing the compiler issue.
Suzuki