Page MenuHomePhabricator

Fix LLDB's lit files
ClosedPublic

Authored by zturner on Nov 14 2018, 9:55 PM.

Details

Summary

Recently I tried to port LLDB's lit configuration files over to use a lot of the common lit infrastructure down in LLVM. This appeared to work on the surface, but broke some cases that weren't broken before and also exposed some additional problems with the old approach that we were just getting lucky with.

This is a big patch, so I'll try to give a high level overview of the problem as best I can to make reviewing easier.

When we set up a lit environment, the goal is to make it as hermetic as possible. We should not be relying on PATH and enabling the use of arbitrary shell commands. Instead, only whitelisted commands should be allowed. These are, generally speaking, the lit builtins such as echo, cd, etc, as well as anything for which substitutions have been explicitly set up for. These substitutions should map to the build output directory, but in some cases it's useful to be able to override this (for example to point to an installed tools directory).

This is, of course, how it's supposed to work. What was actually happening is that we were bringing in PATH and LD_LIBRARY_PATH and then just running the given run line as a shell command. This led to problems such as finding the wrong version of clang-cl on PATH since it wasn't even a substitution, and flakiness / non-determinism since the environment the tests were running in would change per-machine. On the other hand, it also made other things possible. For example, we had some tests that were explicitly running cl.exe and link.exe instead of clang-cl and lld-link and the only reason it worked at all is because it was finding them on PATH. Unfortunately we can't entirely get rid of these tests, because they support a few things in debug info that clang-cl and lld-link don't (notably, the LF_UDT_MOD_SRC_LINE record which makes some of the tests fail.

This patch attempts to fix all of this. It's a large patch, but I'll try to summarize the changes:

  1. Removal of functionality - The lit test suite no longer respects LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER. This means there is no more support for gcc, but nobody was using this anyway (note: The functionality is still there for the dotest suite, just not the lit test suite). There is no longer a single substitution %cxx and %cc which maps to <arbitrary-compiler>, you now explicitly specify the compiler with a substitution like %clang or %clangxx or %clang_cl. We can revisit this in the future when someone needs gcc.
  1. Introduction of the LLDB_LIT_TOOLS_DIR directory. This does in spirit what LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER used to do, but now more friendly. If this is not specified, all tools are expected to be the just-built tools. If it is specified, the tools which are not themselves being tested but are being used to construct and run checks (e.g. clang, FileCheck, llvm-mc, etc) will be searched for in this directory first, then the build output directory.
  1. Changes to core llvm lit files. The use_lld() and use_clang() functions were introduced long ago in anticipation of using them in lldb, but since they were never actually used anywhere but their respective problems, there were some issues to be resolved regarding generality and ability to use them outside their project.
  1. Changes to .test files - These are all just replacing things like clang-cl with %clang_cl and %cxx with %clangxx, etc.
  1. Changes to lit.cfg.py - Previously we would load up some system environment variables and then add some new things to them. Then do a bunch of work building out our own substitutions. First, we delete the system environment variable code, making the environment hermetic. Then, we refactor the substitution logic into two separate helper functions, one which sets up substitutions for the tools we want to test (which must come from the build output directory), and another which sets up substitutions for support tools (like compilers, etc).
  1. New substitutions for MSVC -- Previously we relied on location of MSVC by bringing in the entire parent's PATH and letting subprocess.Popen just run the command line. Now we set up real substitutions that should have the same effect. We use PATH to find them, and then look for INCLUDE and LIB to construct a substitution command line with appropriate /I and /LIBPATH: arguments. The nice thing about this is that it opens the door to having separate %msvc-cl32 and %msvc-cl64 substitutions, rather than only requiring the user to run vcvars first. Because we can deduce the path to 32-bit libraries from 64-bit library directories, and vice versa. Without these substitutions this would have been impossible.

Stella and Aleksandr, can you apply this patch locally and see if it fixes the issues you wer encountering?

Diff Detail

Repository
rC Clang

Event Timeline

zturner created this revision.Nov 14 2018, 9:55 PM
aprantl added a subscriber: davide.Nov 15 2018, 8:39 AM

Removal of functionality - The lit test suite no longer respects LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER. This means there is no more support for gcc, but nobody was using this anyway (note: The functionality is still there for the dotest suite, just not the lit test suite).

The "nobody is using this part" of that statement is just not true.

On green dragon, we have two bots that use LLDB_TEST_C_COMPILER / LLDB_TEST_CXX_COMPILER that run the LLDB testsuite against older versions of clang:
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-6.0.1/

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.

There is also this bot that does something similar with even more compilers, including gcc:

http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31242

Do you happen to know who maintains it?

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.

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.

Removal of functionality - The lit test suite no longer respects LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER. This means there is no more support for gcc, but nobody was using this anyway (note: The functionality is still there for the dotest suite, just not the lit test suite).

The "nobody is using this part" of that statement is just not true.

On green dragon, we have two bots that use LLDB_TEST_C_COMPILER / LLDB_TEST_CXX_COMPILER that run the LLDB testsuite against older versions of clang:
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-6.0.1/

It's possible I didn't make this part clear enough. I didn't mean that nobody is using LLDB_TEST_C_COMPILER, I meant that nobody is using it in order to compile inferiors with gcc. There is also a dichotomy between what happens for the dotest suite and the lit suite. For the dotest suite, LLDB_TEST_C(XX)_COMPILER are still respected, this patch hasn't changed that. It has only changed the behavior of running tests in lldb/lit/{Breakpoint/Expr/Modules/Quit/Settings/SymbolFile/tools}. Even for those tests, it is still possible to use a not-just-built clang, just that instead of specifying LLDB_TEST_C(XX)_COMPILER, you now specify LLDB_LIT_TOOLS_DIR.

aprantl added a comment.EditedNov 15 2018, 9:05 AM

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.

Perhaps I'm misunderstanding something. My primary point is that we need a way to run configure which C and C++ compiler is used to compile the tests in LLDB testsuite. As long as this is still possible, I'm happy.

We discussed previously that we might want to separate tests that exercise different compilers from tests that exercise core functionality of LLDB and perhaps the lit/ vs. the packages/ subdirectories is where we want to draw this line, and only keep the configurability of the compiler for the tests in the packages/ directory. But it isn't clear to me that that is where this is going either.

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.

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.

Perhaps I'm misunderstanding something. My primary point is that we need a way to run configure which C and C++ compiler is used to compile the tests in LLDB testsuite. As long as this is still possible, I'm happy.

We discussed previously that we might want to separate tests that exercise different compilers from tests that exercise core functionality of LLDB and perhaps the lit/ vs. the packages/ subdirectories is where we want to draw this line, and only keep the configurability of the compiler for the tests in the packages/ directory. But it isn't clear to me that that is where this is going either.

I always have trouble making sense of the Greendragon bots. Starting from this page:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/

What do I need to click on to get the equivalent of this: http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31244/steps/test6/logs/stdio

It's possible I didn't make this part clear enough. I didn't mean that nobody is using LLDB_TEST_C_COMPILER, I meant that nobody is using it in order to compile inferiors with gcc. There is also a dichotomy between what happens for the dotest suite and the lit suite. For the dotest suite, LLDB_TEST_C(XX)_COMPILER are still respected, this patch hasn't changed that. It has only changed the behavior of running tests in lldb/lit/{Breakpoint/Expr/Modules/Quit/Settings/SymbolFile/tools}. Even for those tests, it is still possible to use a not-just-built clang, just that instead of specifying LLDB_TEST_C(XX)_COMPILER, you now specify LLDB_LIT_TOOLS_DIR.

Thanks, that makes more sense to me! (I'm still not sure that it is correct though: cf. http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31242)

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.

Do we document the fact that the two testsuites behave differently in this respect? I'm worried that developers that aren't aware of that difference will test new functionality only with a LIT test out of convenience (because they are more familiar with this style from LLVM) and that that will erode our test coverage on other or older compilers over time.

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.

Click on a recent build and then on "Console Output". Here is one from yesterday (we are upgrading the machines right now and temporarily broke the build):
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/1356/consoleFull

Click on a recent build and then on "Console Output". Here is one from yesterday (we are upgrading the machines right now and temporarily broke the build):
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/1356/consoleFull

Ok, now I see it:

python /opt/llvm/zorg/zorg/jenkins//build.py --assertions --cmake-type=Release --cmake-flag=-DLLVM_TARGETS_TO_BUILD=X86 --cmake-flag=-DLLDB_TEST_USE_CUSTOM_C_COMPILER=On --cmake-flag=-DLLDB_TEST_USE_CUSTOM_CXX_COMPILER=On --cmake-flag=-DLLDB_TEST_C_COMPILER=/Users/buildslave/jenkins/workspace/lldb-cmake-clang-5.0.2/lldb-build/history/clang-5.0.2/bin/clang --cmake-flag=-DLLDB_TEST_CXX_COMPILER=/Users/buildslave/jenkins/workspace/lldb-cmake-clang-5.0.2/lldb-build/history/clang-5.0.2/bin/clang++ --dotest-flag=--skip-category --dotest-flag=gmodules lldb-cmake

It's possible I didn't make this part clear enough. I didn't mean that nobody is using LLDB_TEST_C_COMPILER, I meant that nobody is using it in order to compile inferiors with gcc. There is also a dichotomy between what happens for the dotest suite and the lit suite. For the dotest suite, LLDB_TEST_C(XX)_COMPILER are still respected, this patch hasn't changed that. It has only changed the behavior of running tests in lldb/lit/{Breakpoint/Expr/Modules/Quit/Settings/SymbolFile/tools}. Even for those tests, it is still possible to use a not-just-built clang, just that instead of specifying LLDB_TEST_C(XX)_COMPILER, you now specify LLDB_LIT_TOOLS_DIR.

Thanks, that makes more sense to me! (I'm still not sure that it is correct though: cf. http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31242)

FWIW this bot appears unmaintained. I can try to contact the owner though. That aside, it looks like this bot directly runs dotest.py and constructs the command line itself (http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31242/steps/test1/logs/stdio). The first few lines of output in the stdio look like this:

++ dotest_args=()
++ dotest_args+=(-A "$arch" -C "$compiler")
++ dotest_args+=(-v -s "logs-$cc_log-$arch")
++ dotest_args+=(-u CXXFLAGS -u CFLAGS)
++ dotest_args+=(--env ARCHIVER=ar --env OBJCOPY=objcopy)
++ appendCommonArgs
++ dotest_args+=(--executable "$buildDir/bin/lldb")
++ dotest_args+=(--filecheck "$buildDir/bin/FileCheck")
++ for c in '"gdb-remote packets"' '"lldb all"'
++ dotest_args+=(--channel "$c")
++ for c in '"gdb-remote packets"' '"lldb all"'
++ dotest_args+=(--channel "$c")
++ for c in '"${categories[@]}"'
++ case "$c" in
++ dotest_args+=(--skip-category "${c#-}")
++ for c in '"${categories[@]}"'
++ case "$c" in

In that case, I don't think it relies on LLDB_TEST_C_COMPILER etc. It definitely does compile inferiors with gcc, but it seems to do it use its own custom scripts to make all this work.

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.

Do we document the fact that the two testsuites behave differently in this respect? I'm worried that developers that aren't aware of that difference will test new functionality only with a LIT test out of convenience (because they are more familiar with this style from LLVM) and that that will erode our test coverage on other or older compilers over time.

For older versions of clang, I think it's still just as easy to run the test suite with that compiler as opposed to the just built compiler. Just stick your clang binaries in a folder and pass -DLLDB_LIT_TOOLS_DIR (we can change the variable names if anyone has ideas for better / clearer names). So any bots which are currently running tests with old versions of clang can pass this flag and that will still provide coverage with older compiler versions.

For other compilers entirely, it would be nice to come up with a solution. I'd like to eventually get rid of the check-lldb target (which constructs a dotest command line inside of CMake and just runs it directly, bypassing all of this) and if that ever happens, we could get rid of the parallel variables, but it would also necessitate solving the other-compilers problem.

Click on a recent build and then on "Console Output". Here is one from yesterday (we are upgrading the machines right now and temporarily broke the build):
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/1356/consoleFull

Ok, now I see it:

python /opt/llvm/zorg/zorg/jenkins//build.py --assertions --cmake-type=Release --cmake-flag=-DLLVM_TARGETS_TO_BUILD=X86 --cmake-flag=-DLLDB_TEST_USE_CUSTOM_C_COMPILER=On --cmake-flag=-DLLDB_TEST_USE_CUSTOM_CXX_COMPILER=On --cmake-flag=-DLLDB_TEST_C_COMPILER=/Users/buildslave/jenkins/workspace/lldb-cmake-clang-5.0.2/lldb-build/history/clang-5.0.2/bin/clang --cmake-flag=-DLLDB_TEST_CXX_COMPILER=/Users/buildslave/jenkins/workspace/lldb-cmake-clang-5.0.2/lldb-build/history/clang-5.0.2/bin/clang++ --dotest-flag=--skip-category --dotest-flag=gmodules lldb-cmake

The equivalent command line after this patch would be to keep all of the -DLLDB_TEST_XXX variables, but to add one more that says -DLLDB_LIT_TOOLS_DIR=/Users/buildslave/jenkins/workspace/lldb-cmake-clang-5.0.2/lldb-build/history/clang-5.0.2/bin.

It will search this path first when looking for clang and clang++ for all of the LLDB tests, but still use the _CXX_COMPILER and _C_COMPILER paths for the LLDB-Suite tests.

At least, that's the intention.

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.

The ubuntu bot doesn't use the cmake configuration at all. It runs the test in 6 different configurations, but CMake gives you exactly 1 configuration, so it can't do things that way. AFAICT the Ubuntu bot adrian linked has 6 different shell scripts that manually construct the proper dotest command lines.

It's possible I didn't make this part clear enough. I didn't mean that nobody is using LLDB_TEST_C_COMPILER, I meant that nobody is using it in order to compile inferiors with gcc. There is also a dichotomy between what happens for the dotest suite and the lit suite. For the dotest suite, LLDB_TEST_C(XX)_COMPILER are still respected, this patch hasn't changed that. It has only changed the behavior of running tests in lldb/lit/{Breakpoint/Expr/Modules/Quit/Settings/SymbolFile/tools}. Even for those tests, it is still possible to use a not-just-built clang, just that instead of specifying LLDB_TEST_C(XX)_COMPILER, you now specify LLDB_LIT_TOOLS_DIR.

Thanks, that makes more sense to me! (I'm still not sure that it is correct though: cf. http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31242)

FWIW this bot appears unmaintained. I can try to contact the owner though. That aside, it looks like this bot directly runs dotest.py and constructs the command line itself (http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31242/steps/test1/logs/stdio). The first few lines of output in the stdio look like this:

Thanks for checking! It would be unfortunate if it is unmaintained, because bots like that one are a very valuable sanity-check against symmetric bugs, where, e.g., someone misinterprets the DWARF spec and then implements the same wrong behavior in both clang and LLDB.

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.

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.

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.

That could work. Part of the existing confusion and complexity comes from the fact that CMake is static up-front configuration but dotest has a lot of args that control the way things run, including multiple compilers and architectures. Trying to expose these through independent CMake variables and building up a command line probably introduces more complexity than it alleviates.

That said, in the interest of not changing too much all at once, it still seems like something that's perhaps better done in a future patch. WDYT?

Personally I don't think we should differentiate between the lit and dotest suite when it comes to using a custom compiler. The separation between the two suits is mostly the result of what framework is easier to write your test is (or which one you're more familiar with). How hard would it be to have one variable that works for both?

That said, in the interest of not changing too much all at once, it still seems like something that's perhaps better done in a future patch. WDYT?

I actually think it would be better now - the people who use the properties for compile the lit tests will have to make a change now anyway and those are most likely the same people that use the properties to compile the lldb suite tests, so they could make a change once and have both working rather than making it a two-step process.

Personally I don't think we should differentiate between the lit and dotest suite when it comes to using a custom compiler. The separation between the two suits is mostly the result of what framework is easier to write your test is (or which one you're more familiar with). How hard would it be to have one variable that works for both?

I'm not sure how hard it would be.

One problem is that dotest supports not just choosing a compiler, but choosing multiple compilers, as well as multiple architectures and it runs the test suite over the cross product of all of these. That's hard to express in CMake. This is why, for example, people end up subverting it entirely for productionizing test suite runs, such as what you see on the ubuntu bot linked earlier. It doesn't even use the CMake variables for running the dotest suite, it just has scripts that build command lines and runs them.

There is another issue I'm aware of, which is that some people's compilers have version numbers embedded in the binary name. Right now the code that uses LLDB_LIT_TOOLS_DIR to find the binaries won't handle these cases, because it looks specifically for clang and clang++, but not, for example, clang-7.0 or clang++-hexagon-7.0.

I do think an iterative approach is better though. This is already a big change and as this thread (and the previous patch which is what I'm trying to fix) shows, people use things in a lot of different ways so changing something has potential for lots of breakage in subtle ways. So I still kind of prefer doing things incrementally, letting people tell me what's broken, and then working on a solution.

We could try to converge on the single LLDB_LIT_TOOLS_DIR approach for both dotest as well as the lit suite, because having one variable with simple semantics is nice. But I think we should worry first about getting to a known good baseline and then working incrementally to make simplifications.

I'm not sure how hard it would be.

One problem is that dotest supports not just choosing a compiler, but choosing multiple compilers, as well as multiple architectures and it runs the test suite over the cross product of all of these. That's hard to express in CMake. This is why, for example, people end up subverting it entirely for productionizing test suite runs, such as what you see on the ubuntu bot linked earlier. It doesn't even use the CMake variables for running the dotest suite, it just has scripts that build command lines and runs them.

Makes sense. Just to be clear, I'm not advocating running a product for the lit suite, just having one option that controls both dotest and lit.

There is another issue I'm aware of, which is that some people's compilers have version numbers embedded in the binary name. Right now the code that uses LLDB_LIT_TOOLS_DIR to find the binaries won't handle these cases, because it looks specifically for clang and clang++, but not, for example, clang-7.0 or clang++-hexagon-7.0.

How is this handled today? Do we have tests that do something like that?

I do think an iterative approach is better though. This is already a big change and as this thread (and the previous patch which is what I'm trying to fix) shows, people use things in a lot of different ways so changing something has potential for lots of breakage in subtle ways. So I still kind of prefer doing things incrementally, letting people tell me what's broken, and then working on a solution.

I'm all for iteration! We just wanna make sure we share the same "end goal".

We could try to converge on the single LLDB_LIT_TOOLS_DIR approach for both dotest as well as the lit suite, because having one variable with simple semantics is nice. But I think we should worry first about getting to a known good baseline and then working incrementally to make simplifications.

I'm worried that the directory approach is incompatible with settings a specific compiler (like gcc).

I'm not sure how hard it would be.

One problem is that dotest supports not just choosing a compiler, but choosing multiple compilers, as well as multiple architectures and it runs the test suite over the cross product of all of these. That's hard to express in CMake. This is why, for example, people end up subverting it entirely for productionizing test suite runs, such as what you see on the ubuntu bot linked earlier. It doesn't even use the CMake variables for running the dotest suite, it just has scripts that build command lines and runs them.

Makes sense. Just to be clear, I'm not advocating running a product for the lit suite, just having one option that controls both dotest and lit.

There is another issue I'm aware of, which is that some people's compilers have version numbers embedded in the binary name. Right now the code that uses LLDB_LIT_TOOLS_DIR to find the binaries won't handle these cases, because it looks specifically for clang and clang++, but not, for example, clang-7.0 or clang++-hexagon-7.0.

How is this handled today? Do we have tests that do something like that?

The tests themselves don't try to do it, usually our tests try to be compiler agnostic, but then people will run the tests with one or more different compilers. I think people use dotest in this manner, by specifying a path to a compiler with a version number in it, but I think it may be all downstream stuff, with no bot coverage. Mostly they do it by specifying LLDB_TEST_C_COMPILER=/path/to/clang-7.0 or something, and dotest is smart enough to figure this out. We could certainly have this same logic in the lit files though once we get to that point.

I do think an iterative approach is better though. This is already a big change and as this thread (and the previous patch which is what I'm trying to fix) shows, people use things in a lot of different ways so changing something has potential for lots of breakage in subtle ways. So I still kind of prefer doing things incrementally, letting people tell me what's broken, and then working on a solution.

I'm all for iteration! We just wanna make sure we share the same "end goal".

We could try to converge on the single LLDB_LIT_TOOLS_DIR approach for both dotest as well as the lit suite, because having one variable with simple semantics is nice. But I think we should worry first about getting to a known good baseline and then working incrementally to make simplifications.

I'm worried that the directory approach is incompatible with settings a specific compiler (like gcc).

I don't think the directory approach is fundamentally incompatible with using non-clang compilers. But it's important to differentiate what the test suite itself supports and what the test suite supports when invoked via a CMake-generated target. That is to say, we don't have to expose all the power of the underlying test suite through CMake. I actually think trying to do so is a mistake, because you're often going to be circumventing it anyway to run the test suite in some special way that wasn't how you configured CMake. dotest.py, for example, takes tons of different command line arguments, many of which are only available if you invoke it directly, as the CMake-generated command line will never use those arguments.

So I think this is similar. You can pass arbitrary parameters to lit when you point it to a test directory, so there's fundamental limitation in terms of what's possible this way. Of course, we don't currently recognize any, and yes it means you can't specify a specific compiler binary (such as GCC anymore), but I'm not entirely sure how useful that is in the first place, because we're talking about a static CMake configuration.

So I think as far as what we expose through CMake, it should be as simple as possible and support the developer use case and the most common buildbot use cases, but more advanced use cases are still supported by directly invoking llvm-lit.py with some special arguments (which we would still need to teach the LLDB lit configuration files to understand).

Hopefully this all makes sense.

Makes sense. Just to be clear, I'm not advocating running a product for the lit suite, just having one option that controls both dotest and lit.

How is this handled today? Do we have tests that do something like that?

The tests themselves don't try to do it, usually our tests try to be compiler agnostic, but then people will run the tests with one or more different compilers. I think people use dotest in this manner, by specifying a path to a compiler with a version number in it, but I think it may be all downstream stuff, with no bot coverage. Mostly they do it by specifying LLDB_TEST_C_COMPILER=/path/to/clang-7.0 or something, and dotest is smart enough to figure this out. We could certainly have this same logic in the lit files though once we get to that point.

I was aware of the dotest stuff (like Adrian mentioned our bots are using that). My question was specific to lit, but I guess nobody is using that functionality there (yet).

I'm all for iteration! We just wanna make sure we share the same "end goal".

I'm worried that the directory approach is incompatible with settings a specific compiler (like gcc).

I don't think the directory approach is fundamentally incompatible with using non-clang compilers. But it's important to differentiate what the test suite itself supports and what the test suite supports when invoked via a CMake-generated target. That is to say, we don't have to expose all the power of the underlying test suite through CMake. I actually think trying to do so is a mistake, because you're often going to be circumventing it anyway to run the test suite in some special way that wasn't how you configured CMake. dotest.py, for example, takes tons of different command line arguments, many of which are only available if you invoke it directly, as the CMake-generated command line will never use those arguments.

I agree. However I believe that *if* we expose it through CMake (for the convenience of dotest and the lldb-dotest wrapper) it should apply to CMake as well, with the notable exception of arguments we pass through the LLDB_DOTEST_ARGS variable.

So I think this is similar. You can pass arbitrary parameters to lit when you point it to a test directory, so there's fundamental limitation in terms of what's possible this way. Of course, we don't currently recognize any, and yes it means you can't specify a specific compiler binary (such as GCC anymore), but I'm not entirely sure how useful that is in the first place, because we're talking about a static CMake configuration.

Yup, I think we're on the same page here, both dotest and lit accept parameters. We already have a mechanism to forward args to dotest, so if we wanted we could hook things up to make a lit argument work with either or both.

So I think as far as what we expose through CMake, it should be as simple as possible and support the developer use case and the most common buildbot use cases, but more advanced use cases are still supported by directly invoking llvm-lit.py with some special arguments (which we would still need to teach the LLDB lit configuration files to understand).

No objections to this.

Hopefully this all makes sense.

It does, thanks!

zturner updated this revision to Diff 174473.Nov 16 2018, 4:01 PM
  • Added an __init__.py to the helper directory. This is required on Python 2 to make the import work.
  • Changed a bunch of %cc substitutions to %clang.
  • Changed the %clang_cl command lines to always pass /c. This fixes an issue where we are unable to automatically locate the installed Visual Studio when running from inside of VS.

After this patch, the test run is clean on OSX and awaiting word on whether it's clean on Windows (it is for me).

Thanks to @stella.stamenova for the help.

stella.stamenova accepted this revision.Nov 16 2018, 7:36 PM

Feel free to check this in. At this point this will only improve the results on Windows and it works correctly on Linux.

lldb/lit/SymbolFile/PDB/class-layout.test
1 ↗(On Diff #174473)

Why only msvc and not system-windows like the rest of the tests?

This revision is now accepted and ready to land.Nov 16 2018, 7:36 PM

Well msvc implies system-windows, but i can have both

This revision was automatically updated to reflect the committed changes.