[PATCH] D147444: [asan][test][win] Port more tests to not use clang-cl on MinGW

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: compiler-rt/test/asan/TestCases/Windows/crt_initializers.cpp:1
-// UNSUPPORTED: target={{.*-windows-gnu}}
-
-// RUN: %clang_cl_asan -Od %s -Fe%t
+// RUN: %if clang-cl %{ %clang_cl_asan -Od %s -Fe%t %} \
+// RUN: %else %{ %clangxx_asan -O0 %s -o %t %}
----------------
MaskRay wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > vitalybuka wrote:
> > > > mstorsjo wrote:
> > > > > alvinhochun wrote:
> > > > > > mstorsjo wrote:
> > > > > > > alvinhochun wrote:
> > > > > > > > vitalybuka wrote:
> > > > > > > > > I believe it would be much cleaner if done substituting: llvm-project/compiler-rt/test/asan/lit.cfg.py:163
> > > > > > > > > 
> > > > > > > > I don't know how one could use substitution for this, since clang and clang-cl use very different command line parameters (especially problematic for the more complex DLL tests which I have yet to port).
> > > > > > > > 
> > > > > > > > Or do you mean using substitution to turn certain `RUN:` commands into comments? Like:
> > > > > > > > 
> > > > > > > > ```
> > > > > > > > // RUN: %if_clang_cl %clang_cl_asan -Od %s -Fe%t
> > > > > > > > // RUN: %if_not_clang_cl %clangxx_asan -O0 %s -o %t
> > > > > > > > ```
> > > > > > > > 
> > > > > > > > ...and conditionally substituting one of `%if_clang_cl` and `%if_not_clang_cl` to be `:` and the other one to be empty?
> > > > > > > You could make one substitution for `-Fe` or `-o ` (note, including the trailing space), another one for `-O0` vs `-Od` etc.
> > > > > > > 
> > > > > > > Sorry if it’s obvious if I haven’t followed closely enough, but would it be possible to port the tests to use the gcc driver syntax even with clang in msvc mode? (I understand that that would be a bigger undertaking though.) Or are these tests executed with cl.exe too, so we need to remain compatible with that syntax anyway?
> > > > > > If we need one substitution per flag, I don't know how many substitutions we will end up having, and I may have difficulty naming them. Some flags may also be incompatible (mingw needs `-Wl,--out-implib` to generate import lib). To me it feels messier than just writing the commands separately.
> > > > > > 
> > > > > > Rewriting to use gcc driver syntax shouldn't be hard, but I am afraid of breaking them due to obscure differences between the two drivers (things like static/dynamic, debug/release crt). I suppose these tests also serve the purpose of testing asan support in the clang-cl driver so perhaps they should continue to use clang-cl.
> > > > > > 
> > > > > > It doesn't look like the tests would work with cl.exe out of the box though.
> > > > > @vitalybuka - Can you elaborate on how you want this to be substituted?
> > > > Per flag, like %fPIC llvm-project/compiler-rt/test/asan/lit.cfg.py:234 ?
> > > at least for frequent stuff
> > > for flags which are in 1-2 tests if/else should be fine
> > > 
> > @phosek @MaskRay Do you have opinion on %if %else in run lines?
> > Maybe I am unfairly biased against them, when they are reasonable way to solve the problem?
> > 
> I'll certainly avoid complex constructs if possible.
> When they provide values, I'll not simply keep them off. 
> 
> `rg '%if target=' compiler-rt/test/` has some examples.
> 
> Bonus: our internal lit runner supports many `%if` constructs.
@MaskRay Yes, all those hits in `compiler-rt/test` are ones added by @alvinhochun recently. The question is mostly here whether we should prefer adding more `%if` conditions in the test (older revision of this patch) or add more substitutions, to make parametrized expansions that work across both clang-cl and mingw.

As an example - D147432 (which this patch depends on) changes RUN lines with `%clang_cl_asan -Od %s -Fe%t` into `%clang_cl_asan %Od %s %Fe%t`, and for mingw mode, `%Od` expands to `-O0` and `%Fe` expands to `-o`. The run lines get a bit more cryptic, but it works. (But for more complex tests it might not be enough with just such substitutions, but actual conditionals might be needed.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147444