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

[Evgeniy Stepanov via Phabricator via llvm-commits]

eugenis accepted this revision.
eugenis added a comment.
This revision is now accepted and ready to land.

LGTM w/ 2 minor comments



================
Comment at: lib/msan/msan.h:340
+    if (!SANITIZER_CAN_FAST_UNWIND)                                            \
+      GetStackTrace(&stack, 1, pc, bp,                                         \
+                    common_flags()->fast_unwind_on_malloc);                    \
----------------
slthakur wrote:
> 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.
Sorry, I meant min().
store_context_size = 0 should keep working.



================
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
----------------
slthakur wrote:
> 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
Yeah, I've completely forgot about this feature. Please add a short comment to the test case to avoid future questions.


================
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")))
----------------
slthakur wrote:
> 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 ?
No, that's fine.



================
Comment at: test/msan/lit.cfg:45
+else:
+  config.substitutions.append( ('CHECK-%unwind', ("CHECK-NON-MIPS-FRAMES")))
----------------
slthakur wrote:
> 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.
agreed.


Repository:
  rL LLVM

https://reviews.llvm.org/D23107