[Compiler-rt][MSan] fix param_tls_limit test

[mohit.bhakkad]

Summary

Looking at __msan_test_shadow definition, we can see that it returns index of first poisioned (uninitialized) member of the argument.

For example consider :

int i[10];
printf("\nBefore init: %d",__msan_test_shadow(&i, sizeof(i)));
i[0] = 7;
printf("\nAfter    init: %d",__msan_test_shadow(&i, sizeof(i)));

will give output:

Before init: 0
After init: 4

So to check for tls limit, I am initializing 801 size buffer till 800th value, and we can see that part which does not fit is considered fully initialized, i.e. instead of 800, __msan_test_shadow will return -1.

Please correct my understanding if its wrong, as I can see test fails on every arch except x86.

[eugenis]

If an argument does not fit in param-tls, it becomes completely unpoisoned - not just the overflow part, but the entire argument.
I suspect that on non-x86 targets the large struct may be passed as several smaller arguments, and then the ones that fit (completely!) will be poisoned.

[dsanders]

In D17129#350594, @eugenis wrote:

If an argument does not fit in param-tls, it becomes completely unpoisoned - not just the overflow part, but the entire argument.
I suspect that on non-x86 targets the large struct may be passed as several smaller arguments, and then the ones that fit (completely!) will be poisoned.

I can confirm that we slice large aggregates into a series of smaller arguments (i32 on o32, i64 on n32/n64) in clang.

test/msan/param_tls_limit.cc

37

Typo: poision

[mohit.bhakkad]

Thanks,

So can we have modified test as a separate test for archs other than x86?

[eugenis]

Maybe you can define PARTIAL_OVERFLOW() for the arguments that start in param-tls and end outside of it. It would be defined the same as OVERFLOW() on x86, and the same as NO_POISON() on other platforms, resulting in a slightly weaker test.

[mohit.bhakkad]

In D17129#354099, @eugenis wrote:

Maybe you can define PARTIAL_OVERFLOW() for the arguments that start in param-tls and end outside of it. It would be defined the same as OVERFLOW() on x86, and the same as NO_POISON() on other platforms, resulting in a slightly weaker test.

Hi @eugenis, I didn't understood your suggestion clearly.
I understand OVERFLOW() and NO_POISON() will have same definition.

In this revision, I initiialized arguments that start in param-tls and end outside of it upto till param-tls size, so they will match condition of OVERFLOW().
Without initialing, they will match NO_OVERFLOW in archs other than x86.

What will be functionality of PARTIAL_OVERFLOW(), and if you want to keep same test for all archs, than should we initialize overflow arguments ?

[eugenis]

Currently this test has OVERFLOW() and NO_OVERFLOW() checks.
Arguments that are entirely under the tls limit are poisoned on all platforms and can be checked with NO_OVERFLOW.
Arguments (at the source level) that are partially over limit are unpoisoned on X86, and partially poisoned on other platforms due to them being splitted into multiple IR arguments. We can check that with a PARTIAL_OVERFLOW() macro, defined differently based on the platform.

Please keep f_many and f_many2 tests, they are valuable.

PARTIALLY_INIT should not be necessary. If an argument partially overflows, at least the last byte of it will be unpoisoned in the callee - test for that.

[mohit.bhakkad]

defining PARTIAL_OVERFLOW to check shadow of arguments greater than tls limit on platforms where big arguments are sliced in smaller ones.

[eugenis]

test/msan/param_tls_limit.cc

33

800 in "&x + 800" would be 800 * sizeof(x).

something like this to test the last byte of x: (char *)(&(x) + 1) - 1

[mohit.bhakkad]

Changed as suggested.

[mohit.bhakkad]

Updating revision with full context patch

[eugenis]

LGTM

test/msan/param_tls_limit.cc

36–37

This one is also PARTIAL_OVERFLOW. Does not really matter since "int", naturally, does not get split.

test/msan/param_tls_limit.cc

 // ParamTLS has limited size. Everything that does not fit is considered fully
 // initialized.
 
 // RUN: %clangxx_msan -O0 %s -o %t && %run %t
 // RUN: %clangxx_msan -fsanitize-memory-track-origins -O0 %s -o %t && %run %t
 // RUN: %clangxx_msan -fsanitize-memory-track-origins=2 -O0 %s -o %t && %run %t
 //
 // AArch64 fails with:
 // void f801(S<801>): Assertion `__msan_test_shadow(&s, sizeof(s)) == -1' failed
 // XFAIL: aarch64
 
 #include <sanitizer/msan_interface.h>
 #include <assert.h>
 
 // This test assumes that ParamTLS size is 800 bytes.
 
-// This test passes poisoned values through function argument list.
-// In case of overflow, argument is unpoisoned.
-#define OVERFLOW(x) assert(__msan_test_shadow(&x, sizeof(x)) == -1)
-// In case of no overflow, it is still poisoned.
-#define NO_OVERFLOW(x) assert(__msan_test_shadow(&x, sizeof(x)) == 0)
+// This test passes partly poisoned values through function argument list.
+
+// argument is initialized
+#define NO_POISON(x) assert(__msan_test_shadow(&x, sizeof(x)) == -1)
+
+// some part of argument is uninitialized
+#define POISON(x) assert(__msan_test_shadow(&x, sizeof(x)) != -1)
+
+#define PARTIALLY_INIT(s) for(int i=0; i < sizeof(s)-1 ; i++) {s.x[i] = 'i';}
 
 template<int N>
 struct S {
   char x[N];
 };
 
-void f100(S<100> s) {
-  NO_OVERFLOW(s);
-}
-
 void f800(S<800> s) {
-  NO_OVERFLOW(s);
+  POISON(s);
 }
 
 void f801(S<801> s) {
-  OVERFLOW(s);
-}
-
-void f1000(S<1000> s) {
-  OVERFLOW(s);
-}
-
-void f_many(int a, double b, S<800> s, int c, double d) {
-  NO_OVERFLOW(a);
-  NO_OVERFLOW(b);
-  OVERFLOW(s);
-  OVERFLOW(c);
-  OVERFLOW(d);
-}
-
-// -8 bytes for "int a", aligned by 8
-// -2 to make "int c" a partial fit
-void f_many2(int a, S<800 - 8 - 2> s, int c, double d) {
-  NO_OVERFLOW(a);
-  NO_OVERFLOW(s);
-  OVERFLOW(c);
-  OVERFLOW(d);
+  // poision only till the size of ParamTLS will get detected
+  NO_POISON(s);
 }
 
 int main(void) {
-  S<100> s100;
   S<800> s800;
   S<801> s801;
-  S<1000> s1000;
-  f100(s100);
+  PARTIALLY_INIT(s800);
+  PARTIALLY_INIT(s801);
   f800(s800);
   f801(s801);
-  f1000(s1000);
-
-  int i;
-  double d;
-  f_many(i, d, s800, i, d);
-
-  S<800 - 8 - 2> s788;
-  f_many2(i, s788, i, d);
   return 0;
 }