Page MenuHomePhabricator

WIP: [LIT] Have lit run the lldb test suite
ClosedPublic

Authored by JDevlieghere on Apr 5 2018, 1:42 PM.

Details

Summary

This is the initial attempt (v1) to run the lldb test suite with lit, using the custom LLDB test format. Here every`Test*.py` is seen as a single test by lit. As suggested by pavel this circumvents lldb-dotest (though the configuration file now contains part of that logic).

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Apr 5 2018, 1:42 PM

This isn't meant to be checked-in as is, however I'm looking for feedback as early as possible.

There are currently two problems with the current diff:

  • ./bin/llvm-lit ../llvm/tools/lldb/lit/Suite/ doesn't work, and I haven't figured out why yet.
  • We'd run the (do)test-suite twice, once as part lit and once as part of check-lldb.
  • Have lldb test format live in lldb repo
labath added a comment.Apr 9 2018, 4:57 AM

This isn't meant to be checked-in as is, however I'm looking for feedback as early as possible.

There are currently two problems with the current diff:

  • ./bin/llvm-lit ../llvm/tools/lldb/lit/Suite/ doesn't work, and I haven't figured out why yet.
  • We'd run the (do)test-suite twice, once as part lit and once as part of check-lldb.

I think this is quite close to a working diff. The "running twice" issue can be fixed easily, I presume, and the rest of the issues I see are just minor details.

For the llvm-lit issue, I think it would be best if it worked like:

llvm-lit  .../lldb/packages/Python/lldbsuite/test

as then (I hope) I could also specify some folder of that and run only the tests under that subfolder. However, I think we could live without that, initially.

lit/Suite/lit.cfg
17 ↗(On Diff #141608)

The testcases symlink thingy will not work on windows. I think you'll have to put the full path here.

lit/Suite/lldbtest.py
45 ↗(On Diff #141608)

I think -p should match agains the full file name (i.e., you should be able to just pass testFile here).

46 ↗(On Diff #141608)

It looks like executeCommand accepts a list of args for a command as well, so it might be best to keep cmd as such.

JDevlieghere marked 2 inline comments as done.
  • Feedback Pavel
  • Makes check-lldb invoke *only* lit.

The only outstanding issue is calling llvm-lit with the path to the test suite. Does anybody with more lit experience know how we can solve this?

lit/Suite/lldbtest.py
46 ↗(On Diff #141608)

Jup, the (now removed) print statement was just to make it easier for me to debug.

davide accepted this revision.Apr 11 2018, 9:41 AM

Sorry for getting in here late. This seems to be a great improvement on the state of the art, and given it's only enabled for the CMake build, I see little harm not going forward with it.
In particular this gives us:

  1. Proper SIGTERM handling. This was a longstanding problem where you hit ctrl+c and dotest keeps spawning process. I tested this locally and can confirm it works
  2. Some tests are failing with timeouts. because lldb sets a timeout. The new strategy sets a timeout of 0 so it kind of makes this problem going away. OTOH, we can set a timeout per-directory in case it's really needed.

FWIW, I'm not particularly worried about not being able to run this on single tests. I used lldb-dotest for the purpose for a while and it works like a charm. I'm inclined to get this in and back port it to our downstream consumer (swift) as it would be of great benefit there too.

This revision is now accepted and ready to land.Apr 11 2018, 9:41 AM
labath accepted this revision.Apr 12 2018, 2:46 AM

This looks good.

Do you have any plans on how will this work with the XCode build (which is kind of a prerequisite to start removing stuff from dotest)?

This looks good.

Do you have any plans on how will this work with the XCode build (which is kind of a prerequisite to start removing stuff from dotest)?

I haven't spend too much time worrying about Xcode yet. Before we can remove anything we'll need to make sure that we have feature parity, at least for the driver part. I think that'll give us enough time to come up with a solution. :-)

This revision was automatically updated to reflect the committed changes.

Hi Jonas,

This change is causing a cmake generation-time error on Windows when using Visual Studio as generator:
-G "Visual Studio 15 2017 Win64" -Thost=x64

The error is:

CMake Error in tools/lldb/test/CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    D:/build/master_win/tools/lldb/test/../lit/Suite/lit.site.cfg


CMake Error in tools/lldb/test/CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    D:/build/master_win/tools/lldb/test/../lit/Suite/lit.site.cfg


CMake Error in tools/lldb/test/CMakeLists.txt:
  Evaluation file to be written multiple times for different configurations
  or languages with different content:

    D:/build/master_win/tools/lldb/test/../lit/Suite/lit.site.cfg

… and so on

Notice everything runs fine when using Ninja as a Generator and clang-cl.exe as compiler:

-G Ninja -DCMAKE_CXX_COMPILER=clang-cl.exe -DCMAKE_C_COMPILER=clang-cl.exe

Would it be possible for you to repro this ?
Let me know if I can help with this.

ted added a subscriber: ted.Apr 20 2018, 8:25 AM

@JDevlieghere I'm seeing the same issue as @alberto_magni . The Visual Studio cmake generator gives errors. We're reverting it internally, but it needs to be fixed or reverted ASAP.

The issue could be this set of code from test/CMakeLists.txt:

configure_file(
  ${CMAKE_CURRENT_SOURCE_DIR}/../lit/Suite/lit.site.cfg.in
  ${CMAKE_CURRENT_BINARY_DIR}/../lit/Suite/lit.site.cfg
  )
file(GENERATE
  OUTPUT
  ${CMAKE_CURRENT_BINARY_DIR}/../lit/Suite/lit.site.cfg
  INPUT
  ${CMAKE_CURRENT_BINARY_DIR}/../lit/Suite/lit.site.cfg
  )

The equivalent from clang/test/CMakeLists.txt is:

configure_lit_site_cfg(
  ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
  ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg.py
  MAIN_CONFIG
  ${CMAKE_CURRENT_SOURCE_DIR}/lit.cfg.py
  )

configure_lit_site_cfg(
  ${CMAKE_CURRENT_SOURCE_DIR}/Unit/lit.site.cfg.py.in
  ${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg.py
  MAIN_CONFIG
  ${CMAKE_CURRENT_SOURCE_DIR}/Unit/lit.cfg.py
  )

Since Visual Studio as a Generator supports multiple build configurations, each of the CMakeLists.txt that correspond to lit tests also need to include handling of the build mode unless you can guarantee that all the properties are set correctly before test/CMakeLists.txt is processed. It would be safer, of course, to not rely on build order and always set the correct properties. This is to create project files that allow the build mode to be specified at build/compile time of the project rather than configuration time when CMake is called.

So you will need to add something along the lines of (Note: I think these are all the properties you will need, but I have not tested the change, so you will need to make sure this covers all the properties):

if (CMAKE_CFG_INTDIR STREQUAL ".")
  set(LLVM_BUILD_MODE ".")
else ()
  set(LLVM_BUILD_MODE "%(build_mode)s")
endif ()

string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_LIBS_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR})
string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TOOLS_DIR ${LLVM_RUNTIME_OUTPUT_INTDIR})

I suspect you will also need to add handling for the custom c/cxx compiler options. You can look at: lldb/lit/lit.site.cfg.in and lldb/lit/CMakeLists.txt for the details.