Page MenuHomePhabricator

[test] Fix various module cache bugs and inconsistencies
ClosedPublic

Authored by JDevlieghere on Aug 29 2019, 11:24 AM.

Details

Summary

Currently, lit tests don't set neither the module cache for building inferiors nor the module cache used by lldb when running tests. Furthermore, we have several places where we rely on the path to the module cache being always the same, rather than passing the correct value around. This makes it hard to specify a different module cache path when debugging a a test.

This patch reworks how we determine and pass around the module cache paths and fixes the omission on the lit side. It also adds a sanity check to the lit and dotest suites.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

JDevlieghere created this revision.Aug 29 2019, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 11:24 AM
JDevlieghere edited the summary of this revision. (Show Details)Aug 29 2019, 11:25 AM
aprantl added inline comments.Aug 29 2019, 11:30 AM
lldb/lit/CMakeLists.txt
18

"The Clang module cache used by the Clang embedded in LLDB while running tests."

19

"The Clang module cache used by the Clang while building tests."

lldb/lit/Settings/TestModuleCacheSanity.test
2

points inside the

4

CHECK: lldb-test-build.noindex{{.*}}module-cache-lldb

lldb/packages/Python/lldbsuite/test/sanity/TestModuleCacheSanity.py
3

see my comment in the other test

17

delete this comment

25

see my comment in the other test

aprantl accepted this revision.Aug 29 2019, 11:32 AM

Thanks, this looks great (few comments inside).

This revision is now accepted and ready to land.Aug 29 2019, 11:32 AM
  • Fix a few inconsistencies.
  • Address Adrian's comments.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 29 2019, 11:39 AM
labath added a subscriber: labath.Aug 29 2019, 11:51 PM
labath added inline comments.
lldb/trunk/lit/Settings/TestModuleCacheSanity.test
1–4 ↗(On Diff #217951)

It seems weird to expose these as user-settable cmake variables, but then have a test which will fail if the user changes them in any way. Were you intending to have this feature be configurable at the cmake level? If not, could you change the cmake cache variables into something else (looks like regular variables should work just fine). If yes, then we should have a different testing strategy..