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 && \ ---------------- vitalybuka wrote: > 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. maybe COMPILER_RT_TEST_COMPILER_CFLAGS is enough Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77249/new/ https://reviews.llvm.org/D77249 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
