[compiler-rt] Fix tests after defaulting to -fno-common. NFC.

[SjoerdMeijer]

Summary

After committing D75056, which I've reverted now, these compiler-rt tests failed:

    
FAIL: AddressSanitizer-aarch64-linux::odr_c_test.c
FAIL: AddressSanitizer-aarch64-linux::set_shadow_test.c

and the same tests in a different configuration:

FAIL: AddressSanitizer-aarch64-linux-dynamic::odr_c_test.c
FAIL: AddressSanitizer-aarch64-linux-dynamic::set_shadow_test.c

I don't want to hold up this commit for these tests, so I intend to recommit D75056 soon'ish with these test changes included. I am posting this for review so that we can (post-commit) further address this.

It looks like these tests are not very robust and rely on -fcommon behaviour. But both tests are not really in my area, so a second opinion would be good to confirm if adding -fcommon to the run lines is good enough, or if the tests need to be rewritten.

[SjoerdMeijer]

I probably forgot to add why these tests started failing!

For compiler-rt/test/asan/TestCases/Linux/odr_c_test.c, expected string CHECK: The following global variable is not properly aligned did not occur anymore. So, global ZZZ is now aligned.

And for the other case:

.../compiler-rt/test/asan/TestCases/set_shadow_test.c:37:10: error: XF1: expected string not found in input
 // XF1: AddressSanitizer: stack-buffer-underflow
              ^

we don't trigger this stack buffer underflow anymore.

[vsk]

Adding reviewers with more familiarity with Asan.

[jyknight]

compiler-rt/test/asan/TestCases/Linux/odr_c_test.c

4–7

The test is explicitly testing the detection of the conflict between the common ZZZ (in FILE1) vs a non-common ZZZ (in FILE2), via the non-matching alignment.

Adding -fcommon here is correct.

compiler-rt/test/asan/TestCases/set_shadow_test.c

1

This test looks like this test wants to pretend the global "a" is on the stack, but when "a" is an actual global, ASAN knows it doesn't need to instrument the store, so the pretending fails.

I think a nicer fix would be this:

@@ -21,13 +21,13 @@
 void __asan_set_shadow_f5(size_t addr, size_t size);
 void __asan_set_shadow_f8(size_t addr, size_t size);
 
-char a __attribute__((aligned(8)));
+char* a;
 
 void f(long arg) {
   size_t shadow_offset;
   size_t shadow_scale;
   __asan_get_shadow_mapping(&shadow_scale, &shadow_offset);
-  size_t addr = (((size_t)&a) >> shadow_scale) + shadow_offset;
+  size_t addr = (((size_t)a) >> shadow_scale) + shadow_offset;
 
   switch (arg) {
   // X00-NOT: AddressSanitizer
@@ -61,9 +61,10 @@
 int main(int argc, char **argv) {
   assert(argc > 1);
 
+  a = malloc(8);
   long arg = strtol(argv[1], 0, 16);
   f(arg);
-  a = 1;
+  *a = 1;
   printf("PASS\n");
   return 0;
 }

[SjoerdMeijer]

Thanks James, I will prepare a new revision!
The test-suite needs porting too (see comment in D75056) and am first looking into that, after that I will return to this.

[SjoerdMeijer]

Thanks for that solution, have copied it to the test file.

[SjoerdMeijer]

I wanted to include these test changes in the recommit, but perhaps it's nice to commit this separately (as this this a change in a different component).
So wanted to check if we are happy with this?

[SjoerdMeijer]

Thanks for taking a look. I had already merged this into the other commit, the one that changed the behaviour of -fcommon. So, this has been committed, closing this change.

compiler-rt/test/asan/TestCases/Linux/odr_c_test.c

 // Test that we can properly report an ODR violation
 // between an instrumented global and a non-instrumented global.
 
-// RUN: %clang_asan %s -fPIC -shared -o %dynamiclib1  -DFILE1
-// RUN: %clang_asan %s -fPIC -shared -o %dynamiclib2  -DFILE2
-// RUN: %clang_asan %s -fPIE %ld_flags_rpath_exe1 %ld_flags_rpath_exe2 -o %t
+// RUN: %clang_asan -fcommon %s -fPIC -shared -o %dynamiclib1  -DFILE1
+// RUN: %clang_asan -fcommon %s -fPIC -shared -o %dynamiclib2  -DFILE2
+// RUN: %clang_asan -fcommon %s -fPIE %ld_flags_rpath_exe1 %ld_flags_rpath_exe2 -o %t
 // RUN: not %run %t 2>&1 | FileCheck %s
 //
 // CHECK: The following global variable is not properly aligned.
 // CHECK: ERROR: AddressSanitizer: odr-violation
 #if defined(FILE1)
 __attribute__((aligned(8))) int x;
 __attribute__((aligned(1))) char y;
 // The gold linker puts ZZZ at the start of bss (where it is aligned)
 // unless we have a large alternative like Displace:

compiler-rt/test/asan/TestCases/set_shadow_test.c

 // RUN: %clang_asan -O0 %s -o %t
 // RUN: %run %t 0x00 2>&1 | FileCheck %s -check-prefix=X00
 // RUN: not %run %t 0xf1 2>&1 | FileCheck %s -check-prefix=XF1
 // RUN: not %run %t 0xf2 2>&1 | FileCheck %s -check-prefix=XF2
 // RUN: not %run %t 0xf3 2>&1 | FileCheck %s -check-prefix=XF3
 // RUN: not %run %t 0xf5 2>&1 | FileCheck %s -check-prefix=XF5
 // RUN: not %run %t 0xf8 2>&1 | FileCheck %s -check-prefix=XF8
 
 // XFAIL: windows-msvc
 
 #include <assert.h>
 #include <sanitizer/asan_interface.h>
 #include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 void __asan_set_shadow_00(size_t addr, size_t size);
 void __asan_set_shadow_f1(size_t addr, size_t size);
 void __asan_set_shadow_f2(size_t addr, size_t size);
 void __asan_set_shadow_f3(size_t addr, size_t size);
 void __asan_set_shadow_f5(size_t addr, size_t size);
 void __asan_set_shadow_f8(size_t addr, size_t size);
 
-char a __attribute__((aligned(8)));
+char* a;
 
 void f(long arg) {
   size_t shadow_offset;
   size_t shadow_scale;
   __asan_get_shadow_mapping(&shadow_scale, &shadow_offset);
-  size_t addr = (((size_t)&a) >> shadow_scale) + shadow_offset;
+  size_t addr = (((size_t)a) >> shadow_scale) + shadow_offset;
 
   switch (arg) {
   // X00-NOT: AddressSanitizer
   // X00: PASS
   case 0x00:
     return __asan_set_shadow_00(addr, 1);
   // XF1: AddressSanitizer: stack-buffer-underflow
   // XF1: [f1]
case 0xf8:
     return __asan_set_shadow_f8(addr, 1);
   }
   assert(0);
 }
 
 int main(int argc, char **argv) {
   assert(argc > 1);
 
+  a = malloc(8);
   long arg = strtol(argv[1], 0, 16);
   f(arg);
-  a = 1;
+  *a = 1;
   printf("PASS\n");
   return 0;
 }