[PATCH] D77249: [MSan] Pass command line options to MSan with new pass manager

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added inline comments.


================
Comment at: compiler-rt/test/msan/chained_origin_empty_stack_npm.cpp:4
+// this test.
+// RUN: %clangxx_msan -fsanitize-memory-track-origins=2 \
+// RUN:     -fexperimental-new-pass-manager -O3 %s -o %t && \
----------------
rnk wrote:
> leonardchan wrote:
> > nemanjai wrote:
> > > nemanjai wrote:
> > > > vitalybuka wrote:
> > > > > Why not to add RUN: section with -fexperimental-new-pass-manager into original tests?
> > > > 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:
> > > > - compile/link
> > > > - run with one option set and FileCheck
> > > > - run with another option set and FileCheck
> > > > - 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.
> > > 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.
> > 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.
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77249/new/

https://reviews.llvm.org/D77249