[Lldb-commits] [PATCH] D69031: [LLDB] [test] Use %clang_cl instead of build.py in a few tests

[Martin Storsjö via Phabricator via lldb-commits]

mstorsjo added a comment.

In D69031#1714143 <https://reviews.llvm.org/D69031#1714143>, @labath wrote:

> In D69031#1714107 <https://reviews.llvm.org/D69031#1714107>, @mstorsjo wrote:
>
> > In D69031#1713900 <https://reviews.llvm.org/D69031#1713900>, @aprantl wrote:
> >
> > > Check out  `lldb/test/Shell/helper/toolchain.py`, you probably need to filter out some options for clang_cl specifically.
> >
> >
> > Yeah, I later found that. The issue is that it's passed to usr_clang in i llvm/utils/lit/lit/llvm/config.py, which sets all the provided additional flags on both %clang, %clangxx, %clang_cc1 and %clang_cl.
> >
> > Maybe the additional_flags param needs to be split, into one common for all, one for gcc-style driver, one for clang_cl, and maybe also a separate one for cc1 (where not all driver level flags may fit either)?
>
>
> Actually, it seems to be that these arguments should not be added to the command line by default. All of the existing tests that do "%clang -target whatever" don't need the arguments, and they "only" work because the arguments do no harm because the tests don't do anything which would depend on them. I think we should leave `additional_flags` argument mostly empty, and instead have a separate way of invoking clang when building for the host. Either %clang_host (achievable with a recursive substitution %clang_host -> %clang <whatever>), or using something like %clang %host_cflags.
>
> I can have a stab at that, if you like, but I'm not sure I'll get around to that before the llvm conference is over...


Ok, that makes sense.

I'd appreciate if you do that. I have no hurry wrt this patch as it's just a collateral fix I picked up along the way :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69031