This is an archive of the discontinued LLVM Phabricator instance.

[MSan] Pass command line options to MSan with new pass manager
Needs ReviewPublic

Authored by nemanjai on Apr 1 2020, 3:57 PM.

Details

Reviewers
eugenis
philip.pfaffe
leonardchan
vitalybuka
Group Reviewers
Restricted Project
Summary

There are a number of test cases that fail when clang is built to use NPM by default. This is due to a number of issues. One of those issues is that when we construct the memory sanitizer instrumentation pass with the NPM, we do not pass in the pertinent flags. This patch focuses on that issue.

For testing, this patch simply duplicates the tests that fail due to this issue and adds -fexperimental-new-pass-manager to the compile step. Once the NPM is the default, we can presumably remove those test cases.

Diff Detail

Event Timeline

nemanjai created this revision.Apr 1 2020, 3:57 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 1 2020, 3:57 PM
Herald added subscribers: Restricted Project, cfe-commits. · View Herald Transcript
vitalybuka added inline comments.Apr 1 2020, 6:27 PM
compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp
4

Why not to add RUN: section with -fexperimental-new-pass-manager into original tests?

nemanjai marked an inline comment as done.Apr 1 2020, 7:26 PM
nemanjai added inline comments.
compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp
4

I just felt that this is a simpler way forward for a couple of reasons:

  1. Once the default switches, it is a very obvious change to just delete these files rather than digging through the code inside the existing ones
  2. Many of the tests actually contain the testing that is split up into multiple steps so I would have to duplicate all the steps for the NPM vs. default builds:
  3. compile/link
  4. run with one option set and FileCheck
  5. run with another option set and FileCheck
  6. rinse/repeat

(example: chained_origin_limits.cpp)

But of course, if there are strong objections to this approach, I can certainly go the other way.

nemanjai marked an inline comment as done.Apr 1 2020, 10:26 PM
nemanjai added inline comments.
compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp
4

Seems Phabricator reformatted what I wrote here. Points 3, 4, 5, 6 were supposed to be sub-bullets for 2.
Basically, I tried to describe that in the mentioned test case, I would have to replicate a number of subsequent steps for each RUN directive that invokes the compiler.

leonardchan added inline comments.Apr 3 2020, 11:39 AM
compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp
4

If we're going this way, I think the original tests should explicitly have -fno-experimental-new-pass-manager. Also no strong preference towards either way.

rnk added a subscriber: rnk.Apr 8 2020, 1:47 PM
rnk added inline comments.
compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp
4

I don't think we should even make changes to the tests in compiler-rt. We should write a targeted test in clang/test/CodeGen that ensures these options are passed down correctly to the MSan instrumentation pass. It should be easy to FileCheck the IR to look for the appropriate instrumentation callbacks. We can run that test with the new and old PM.

vitalybuka added inline comments.May 6 2020, 1:55 PM
clang/lib/CodeGen/BackendUtil.cpp
1232

this patch changes behavior of clang CodeGen, so it should have tests there.
probably in a test similar to clang/test/CodeGen/asan-new-pm.ll

vitalybuka added inline comments.May 6 2020, 1:59 PM
compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp
4

I also prefer not have these new tests.
Maybe we can add some option to run all compiler-rt tests with new PM. It should be a separate patch.

vitalybuka added inline comments.May 9 2020, 1:12 AM
compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp
4

maybe COMPILER_RT_TEST_COMPILER_CFLAGS is enough

jsji added a subscriber: jsji.May 11 2020, 1:46 PM

NOTE that https://reviews.llvm.org/D79445 has committed a similar fix and fixed most of the failures.

vitalybuka resigned from this revision.Jul 6 2020, 9:53 PM