[hwasan] Fix and re-enable deep-recursion.c #69265
Conversation
deep-recursion.c was disabled (llvm@c007e0f) because the test may get unlucky and end up with a zero-tagged variable, leading to a false negative (llvm#69221). This patch re-enables the test and adds a workaround: it checks if the variable is zero-tagged, and if so, it will instead use the neighboring variable, which must have a different (hence non-zero) tag. Fixing the stack allocation tagging is left as an exercise for the reader. It is non-trivial because, even if the stackTagBase is non-zero, tags for subsequent allocations in the stack frame may wrap around to zero; working around this would require adding multiple instructions to each alloca.
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Thurston Dang (thurstond) Changesdeep-recursion.c was disabled This patch re-enables the test and adds a workaround: it checks Fixing the stack allocation tagging is left as an exercise for the Full diff: https://github.com/llvm/llvm-project/pull/69265.diff 1 Files Affected:
diff --git a/compiler-rt/test/hwasan/TestCases/deep-recursion.c b/compiler-rt/test/hwasan/TestCases/deep-recursion.c
index bf390d051d472e7..c992f205917ba22 100644
--- a/compiler-rt/test/hwasan/TestCases/deep-recursion.c
+++ b/compiler-rt/test/hwasan/TestCases/deep-recursion.c
@@ -17,9 +17,6 @@
// Stack histories are currently not recorded on x86.
// XFAIL: target=x86_64{{.*}}
-// Flaky on AArch64 Linux, see https://github.com/llvm/llvm-project/issues/69221.
-// UNSUPPORTED: target=aarch64{{.*}}
-
#include <stdlib.h>
// At least -O1 is needed for this function to not have a stack frame on
// AArch64.
@@ -29,7 +26,23 @@ void USE(void *x) { // pretend_to_do_something(void *x)
volatile int four = 4;
-__attribute__((noinline)) void OOB() { int x[4]; x[four] = 0; USE(&x[0]); }
+__attribute__((noinline)) void OOB() {
+ int x[4];
+ int y[4];
+
+ // Tags for stack-allocated variables can occasionally be zero, resulting in
+ // a false negative for this test. This is not easy to fix, hence we work
+ // around it: if the tag is zero, we use the neighboring variable instead,
+ // which must have a different (hence non-zero) tag.
+ // This tag check assumes aarch64.
+ if(((unsigned long)&x) >> 56 == 0) {
+ y[four] = 0;
+ } else {
+ x[four] = 0;
+ }
+ USE(&x[0]);
+ USE(&y[0]);
+}
__attribute__((noinline)) void FUNC1() { int x; USE(&x); OOB(); }
__attribute__((noinline)) void FUNC2() { int x; USE(&x); FUNC1(); }
__attribute__((noinline)) void FUNC3() { int x; USE(&x); FUNC2(); }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
The latest PR update is pulling in unrelated commits. Setting this PR to draft until I sort it out. |
| // around it: if the tag is zero, we use the neighboring variable instead, | ||
| // which must have a different (hence non-zero) tag. | ||
| // This tag check assumes aarch64. | ||
| if(((unsigned long)&x) >> 56 == 0) { |
There was a problem hiding this comment.
Would this be better as uintptr_t?
There was a problem hiding this comment.
Good point, fixed!
thurstond
force-pushed
the
we_must_go_deeper
branch
from
f1845b4 to
a209d08
Compare
|
I think this is still marked as a draft. I'm not sure if that was intentional or not. |
Oops, I forgot to explicitly toggle it. Thanks for pointing it out! |
|
I will land so we can see results by tomorrow,. |
Thank you Vitaly! |
deep-recursion.c was disabled
(c007e0f)
because the test may get unlucky and end up with a zero-tagged variable, leading to a false negative (#69221).
This patch re-enables the test and adds a workaround: it checks
if the variable is zero-tagged, and if so, it will instead use the
neighboring variable, which must have a different (hence non-zero)
tag.
Fixing the stack allocation tagging is left as an exercise for the
reader. It is non-trivial because, even if the stackTagBase is
non-zero, tags for subsequent allocations in the stack frame may wrap
around to zero; working around this would require adding multiple
instructions to each alloca.