[PATCH] D23107: [MSAN][MIPS] Fix fork.cc test on MIPS

[Sagar Thakur via Phabricator via llvm-commits]

slthakur added inline comments.


================
Comment at: lib/msan/msan.h:340
+    if (!SANITIZER_CAN_FAST_UNWIND)                                            \
+      GetStackTrace(&stack, 1, pc, bp,                                         \
+                    common_flags()->fast_unwind_on_malloc);                    \
----------------
eugenis wrote:
> max(1, flags()->store_context_size)
max(1, flags()->store_context_size) will return the value of flags()->store_context_size which is 1000 in the test case fork.cc. This will invoke the slow unwinder and cause deadlock.


================
Comment at: test/msan/chained_origin_limits.cc:157
+// CHECK-NON-MIPS-FRAMES-PER-STACK: Uninitialized value was stored to memory at
+// CHECK-NON-MIPS-FRAMES-PER-STACK: in fn1
 // CHECK-PER-STACK: Uninitialized value was created
----------------
eugenis wrote:
> Hm, this test expects three instances of "Uninitialized value was stored to memory at" on non-mips, and only one instance on mips. Why is that?
> 
On architectures with short stack, due to reduced origin context, all the stacks in the chain are same because the stack trace contains only 1 frame and does not contain frames upto the functions (fn1, fn2, fn3) from where the uninitialized stores actually originate. And since origin_history_per_stack_limit is set to 1 we only get 1 instance of "Uninitialized value was stored to memory at" per stack. Therefore even though the uninitialized stores come from 3 different functions we only have one stack trace. In case of architectures having full stack, there are 3 different stacks because the uninitialized stores originate from 3 different functions in the chain and therefore we can see 1  instance (per stack) of "Uninitialized value was stored to memory at".

The full stack (for architectures with full origin context) which is different from the 2 other stacks in the chain is :

>Uninitialized value was stored to memory at
>    #0 0xaaaef00f10 in __msan_memmove /home/slt/LLVM/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:1480:3
>    #1 0xaaaef9ccd4 in line_flush /home/slt/LLVM/llvm/projects/compiler-rt/test/msan/chained_origin_limits.cc:90:5
>    #2 0xaaaef9ccd4 in buffered_write /home/slt/LLVM/llvm/projects/compiler-rt/test/msan/chained_origin_limits.cc:102
>    #3 0xaaaef9ccd4 in fn3 /home/slt/LLVM/llvm/projects/compiler-rt/test/msan/chained_origin_limits.cc:115
>    #4 0xaaaef9ccd4 in main /home/slt/LLVM/llvm/projects/compiler-rt/test/msan/chained_origin_limits.cc:123

Whereas the short stack (for architectures with short origin context) which is same as all other stacks in the chain is :

>Uninitialized value was stored to memory at
>    #0 0xaab04dcf20 in __msan_memmove /home/slt/LLVM/llvm/projects/compiler-rt/lib/msan/msan_interceptors.cc:1480:3


================
Comment at: test/msan/lit.cfg:42
+# check the first frame since store_context size is 1.
+if config.host_arch in ['mips64', 'mips64el']:
+  config.substitutions.append( ('CHECK-%unwind', ("CHECK-MIPS-FRAMES")))
----------------
eugenis wrote:
> fast unwinder is also unavailable on sparc, please include that here
Msan is not supported on sparc architecture. Do you still want me to include it ?


================
Comment at: test/msan/lit.cfg:45
+else:
+  config.substitutions.append( ('CHECK-%unwind', ("CHECK-NON-MIPS-FRAMES")))
----------------
eugenis wrote:
> Instead of mips and non-mips, let's call it something neutral, like
> %short-stack -> "SHORT-STACK" on mips and sparc, and "" on other platforms.
> Then any affected CHECK can be given a -%short-stack suffix.
If we use ""  in place of %short-stack for architectures which have full stack, then we will have to check the output lines for the full stack with the "CHECK" prefix . And these full stack frame lines will also be checked for architectures which have short stack because most of the tests have "--check-prefix=CHECK" for checking output lines other than stack frames.

Therefore it is important to distinguish between architectures having short stack and architectures having full stack context. So I have used
%short-stack -> "CHECK-FULL-STACK" substitution for architectures having full stack size. Please let me know if this should be done in a different way.


Repository:
  rL LLVM

https://reviews.llvm.org/D23107