[Lldb-commits] [PATCH] D54567: Fix LLDB's lit files

[Stella Stamenova via Phabricator via lldb-commits]

stella.stamenova added a comment.

In https://reviews.llvm.org/D54567#1300064, @zturner wrote:

> In https://reviews.llvm.org/D54567#1300029, @stella.stamenova wrote:
>
> > In https://reviews.llvm.org/D54567#1299999, @zturner wrote:
> >
> > > In https://reviews.llvm.org/D54567#1299993, @stella.stamenova wrote:
> > >
> > > > In https://reviews.llvm.org/D54567#1299992, @aprantl wrote:
> > > >
> > > > > In https://reviews.llvm.org/D54567#1299989, @stella.stamenova wrote:
> > > > >
> > > > > > We should also remove LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER from the cmake files along with this change, otherwise, people will still expect them to work.
> > > > >
> > > > >
> > > > > That would not be a good idea. There are several bots that are using these flags.
> > > >
> > > >
> > > > The change that Zachary is making is removing their usage, so after his change they would not do anything. If he ends up committing this change, these two properties (along with LLDB_DEFAULT_TEST_C_COMPILER and LLDB_TEST_USE_CUSTOM_C_COMPILER, etc.) should also go.
> > >
> > >
> > > The flags are still needed for (and used by) the dotest suite, I didn't change that part.  Normally you run that suite by doing `ninja check-lldb`, in which case it never goes through these lit files to begin with.  But they will also run as part of `ninja check-lldb-lit`, but that lit configuration file totally overrides everything in the parent one, so nothing in this patch should have any effect on that.
> >
> >
> > I think this is actually confusing - there are two ways to specify compilers for the lldb test suite at cmake time:
> >
> > 1. Via LLDB_TEST_USE_CUSTOM_C_COMPILER and friends
> > 2. Via LLDB_TEST_USER_ARGS
> >
> >   As far as I can tell, the ubuntu 14 bot that @aprantl pointed to uses the LLDB_TEST_USER_ARGS path. I *think* the green dragon bots also use the LLDB_TEST_USER_ARGS and AFAIK it's the only way gcc is ever specified for the tests. If the LLDB_TEST_USE_CUSTOM_C_COMPILER and friends are not used for the lit tests, I don't think we need them for the suite tests.
>
>
> You're right that it's confusing, but I don't have any good ideas here.  Some ideas (which are not necessarily good):
>
> - We could change the names of `LLDB_TEST_C_COMPILER` and `LLDB_TEST_CXX_COMPILER` to `LLDB_DOTEST_C_COMPILER` and `LLDB_DOTEST_CXX_COMPILER`.
> - We could restructure the directories under `lldb/lit` so that it looks like this:
>
>   ``` lldb \- lit |- tests |- unittests \- dotest ```
> - We could get rid of the lit folder entirely, and put everythign in the tests folder, like this: ``` lldb \- test |- lit |- unittests \- dotest ```
>
>   Maybe some combinatino of these (or other ideas that others have) can make things clearer.  But I don't want to change too much all at once.


I actually think we should remove it entirely and rely on LLDB_USER_TEST_ARGS because it allows for the user to specify exactly which compiler to use for the lldb suite and it only gives the user one way to do it (instead of two which magically get merged for the lldb suite).

In the cmake for the tests, this is what we do:

  list(APPEND LLDB_TEST_COMMON_ARGS
    --executable ${LLDB_TEST_EXECUTABLE}
    --dsymutil ${LLDB_TEST_DSYMUTIL}
    --filecheck ${LLDB_TEST_FILECHECK}
    -C ${LLDB_TEST_C_COMPILER}
    )

so setting the LLDB_TEST_C_COMPILER is the equivalent of passing ```-c \path\to\compiler``` as part of the LLDB_TEST_USER_ARGS. Also, we don't actually use the LLDB_TEST_CXX_COMPILER for the lldb suite at all.


https://reviews.llvm.org/D54567