Page MenuHomePhabricator

[lldb] Remove LLDB session dir and just store test traces in the respective test build directory
ClosedPublic

Authored by teemperor on Dec 2 2020, 11:33 AM.

Details

Summary

Test runs log some of their output to files inside the LLDB session dir. This session dir is shared between
all tests, so all the tests have to make sure they choose a unique file name inside that directory. We
currently choose by default <test-class-name>-<test-method-name> as the log file name. However,
that means that if not every test class in the test suite has a unique class name, then we end up with a
race condition as two tests will try to write to the same log file.

I already tried in D83767 changing the format to use the test file basename instead (which we already require to be unique
for some other functionality), but it seems the code for getting the basename didn't work on Windows.

This patch instead just changes that dotest stores the log files in the build directory for the current test. We
know that directory is unique for this test, so no need to generate some unique file name now. Also removes
all the environment vars and parameters related to the now unused session dir.

The new log paths now look like this for a failure in 'TestCppOperators`:

./lldb-test-build.noindex/lang/cpp/operators/TestCppOperators.test_dwarf/Failure.log
./lldb-test-build.noindex/lang/cpp/operators/TestCppOperators.test_dsym/Failure.log
./lldb-test-build.noindex/lang/cpp/operators/TestCppOperators.test_gmodules/Failure.log

Diff Detail

Event Timeline

teemperor created this revision.Dec 2 2020, 11:33 AM

Personally I like this. It's in line with the effort of making sure test runs are self contained in the build directory and it mimics what the shell test do.

I like this too, modulo the inline comment.

lldb/examples/test/.lldb-loggings
1

Wow? What is this? does it even work?

lldb/packages/Python/lldbsuite/test/lldbtest.py
1169

IIUC, in case of Prefix=None (used as "temporary" names before the test result is known) this will produce a file in the _parent_ directory of the current build dir (with a name similar to the build dir).

That's not ideal because the temporary names can leak out, for example when the test crashes. Maybe we could name these "Incomplete.log" or something ?

teemperor updated this revision to Diff 309475.Dec 4 2020, 12:53 AM
  • Use a default file prefix to prevent log creation in the parent directory if the caller didn't specify a prefix.
teemperor added inline comments.Dec 4 2020, 12:54 AM
lldb/examples/test/.lldb-loggings
1

Apparently that's some script that is supposed to split the logs by test modes (like, gmodules/dsym/dwarf)? We're already doing that by default from what I can know, so not sure if it's useful. I can make a follow up where we remove all of this stuff.

labath accepted this revision.Dec 4 2020, 1:01 AM

Awesome.

lldb/examples/test/.lldb-loggings
1

Yeah, I think that would be great.

This revision is now accepted and ready to land.Dec 4 2020, 1:01 AM
teemperor updated this revision to Diff 309485.Dec 4 2020, 1:47 AM

I was a bit too quick with updating that one, doesn't seem to pass the tests. Two small fixes:

  • Create the build directory *before* creating the initial log file.
  • Fix one call that explicitly passed the old "None" default arg to getLogBasenameForCurrentTest.
teemperor requested review of this revision.Dec 4 2020, 1:47 AM
labath accepted this revision.Dec 4 2020, 2:42 AM
This revision is now accepted and ready to land.Dec 4 2020, 2:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2020, 2:43 AM