This is an archive of the discontinued LLVM Phabricator instance.

Compile the LLDB tests out-of-tree
ClosedPublic

Authored by aprantl on Jan 18 2018, 7:27 PM.

Details

Summary

This patch is the result of a discussion on lldb-dev, see http://lists.llvm.org/pipermail/lldb-dev/2018-January/013111.html for background.

This is a first sketch of how to move building of the testcases in the LLDB testsuite out of the source tree. The patch is buggy and incomplete, but I would like to develop this as much in the open as possible to solicit early feedback in case I'm unwittingly cutting any corners that will break somebody's platform.

For each test (should be eventually: each test configuration) a separate build directory is created and we execute make VPATH=$srcdir/path/to/test -C $builddir/path/to/test -f $srcdir/path/to/test/Makefile -I $srcdir/path/to/test.

In order to make this work all LLDB tests need to be updated to find the executable in the test build directory, since CWD still points at the test's source directory, which is a requirement for unittest2. That patch is very boring and therefore outsourced to https://reviews.llvm.org/D42280 to make this review easier to read.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Jan 18 2018, 7:27 PM
zturner added inline comments.
packages/Python/lldbsuite/test/dotest.py
625 ↗(On Diff #130546)

Here this has the possibility of setting os.environ["LLDB_BUILD"] = None. Is this desired?

1193 ↗(On Diff #130546)

Nothing appears to use orig_working_dir, and we never set a new working directory anyway, so it doesn't seem necessary to save the original working directory. Can this line be removed?

1198 ↗(On Diff #130546)

This relates to the environment variable comment earlier. The code that sets the environment variable gets run before this. So that could set the environment variable to None, and then this code could set configuration.test_build_dir to something valid. It seems to me like we should ensure that the environment variable always gets set to *something*, then the multiprocessing children can just assume it has a valid value.

packages/Python/lldbsuite/test/lldbtest.py
2246 ↗(On Diff #130546)

Related to previous comments about the environment variable, can we just assume the variable is set here?

packages/Python/lldbsuite/test/plugins/builder_base.py
53 ↗(On Diff #130546)

Can you call this maybe test_subdir or something, and update the docstring to indicate what it actually is?

62 ↗(On Diff #130546)

Related to my previous comment about environment variables, but you are assuming it exists here, so it does seem like we can assume it exists in the other codepath too.

216 ↗(On Diff #130546)

Can this be deleted?

labath added a subscriber: labath.Jan 19 2018, 2:33 AM

I like the direction this is going in. When looking at this, it occurred to me some of our tests do recursive make invocations, generally to build shared libraries (TestConflictingSymbol is a good example). The $(MAKE) line in those may need to be fixed somehow.

packages/Python/lldbsuite/test/dotest.py
1195 ↗(On Diff #130546)

Can we just not specify the mode and let user's umask do the selection. That's the standard behaviour of all unix tools.

packages/Python/lldbsuite/test/plugins/builder_base.py
69 ↗(On Diff #130546)

I like this trick. :)
Originally, I was afraid we'd have to copy the Makefile to the build folder or something...

Looks like a good start. It might be nice to validate that after "clean" that we have no files that are untracked in the test directory? This could help us to verify that we don't leave artifacts around.

packages/Python/lldbsuite/test/dotest_args.py
166 ↗(On Diff #130546)

Maybe add a default right here?:

default=os.getcwd()

Looks like a good start. It might be nice to validate that after "clean" that we have no files that are untracked in the test directory? This could help us to verify that we don't leave artifacts around.

My idea was actually to remove the requirement that Makefiles have to implement a functional clean: rule, since we now can clean by merely wiping --build-dir. What do you think about that?

davide added a subscriber: davide.Jan 19 2018, 9:07 AM

Looks like a good start. It might be nice to validate that after "clean" that we have no files that are untracked in the test directory? This could help us to verify that we don't leave artifacts around.

My idea was actually to remove the requirement that Makefiles have to implement a functional clean: rule, since we now can clean by merely wiping --build-dir. What do you think about that?

I think that's a much more robust solution. We had a bunch of issues with clean in the past.

davide requested changes to this revision.Jan 19 2018, 9:09 AM

This looks like a decent way of going forward, I'm a little concerned about swallowing an error, see comment inline.

packages/Python/lldbsuite/test/dotest.py
1196 ↗(On Diff #130546)

should we actually print some warning here? (e.g. directory blah/ already exists?)

This revision now requires changes to proceed.Jan 19 2018, 9:09 AM
aprantl updated this revision to Diff 131103.Jan 23 2018, 10:21 AM

Updating with my current work in progress to show that some progress is being made.
Note that this does not yet address everybody's feedback.

It's currently standing at

Failure:              69
Error:               189

and I'm slowly working through the remaining tests to see why they fail.

aprantl added inline comments.Jan 23 2018, 10:23 AM
packages/Python/lldbsuite/test/dotest_args.py
166 ↗(On Diff #130546)

The default is in dotest.py:474. Let me know if this file is the more appropriate place for it.

aprantl updated this revision to Diff 131173.Jan 23 2018, 5:10 PM

This is starting to look manageable.

Failure:              30
Error:               113
aprantl updated this revision to Diff 131484.Jan 25 2018, 10:50 AM

Time for another status update:

===================
Test Result Summary
===================
Test Methods:       2694
Reruns:                0
Success:            2213
Expected Failure:     89
Failure:              22
Error:                29
Exceptional Exit:      0
Unexpected Success:   12
Skip:                329
Timeout:               0
Expected Timeout:      0
aprantl updated this revision to Diff 131525.Jan 25 2018, 5:37 PM
aprantl marked 7 inline comments as done.

This version actually passes all tests on Darwin. Time to give this a thorough review now!
@zturner: Would you be able to give this a try on Windows and let me know if there are any big problems that I overlooked?

thanks!

jingham requested changes to this revision.Jan 25 2018, 6:39 PM

This looks good to me. I think it would be cleaner if there were a getSourceFileSpec equivalent to getBuildArtifact.

I had a few inline trivial questions.

Also, before you forget all the things you had to do, you need to write down the new rules in the README.testsuite. Might also be good to put a brief note in the sample_test testcases, since I imagine folks are just going to copy that and put it somewhere to make new tests, so reminding them there might not be a bad idea.

packages/Python/lldbsuite/test/api/check_public_api_headers/TestPublicAPIHeaders.py
46 ↗(On Diff #131525)

Why do you have to call getBuildArtifact on the main source file?

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_set_restart/TestBreakpointSetRestart.py
35–37 ↗(On Diff #131525)

This is beginning to show up a bunch. Maybe TestBase should have:

def getSourceFileSpec( filename):
  fileSpec = lldb.SBFileSpec()
  fileSpec.SetDirectory(self.getSourceDir())
  fileSpec.SetFilename(filename)
  return fileSpec
packages/Python/lldbsuite/test/functionalities/exec/TestExec.py
47–48 ↗(On Diff #131525)

Maybe also a TestBase getSourcePath(filename) that returns a string? That might be easier to read.

packages/Python/lldbsuite/test/plugins/builder_base.py
224–232 ↗(On Diff #131525)

Probably can delete this now...

packages/Python/lldbsuite/test/types/AbstractBase.py
37–38 ↗(On Diff #131525)

Why isn't this getBuildArtifact("golden-output.txt")?

This revision now requires changes to proceed.Jan 25 2018, 6:39 PM

I've created D42572 with the fixups necessary to make this run on linux. It's way smaller than I expected we would need -- I basically fixed one typo and corrected for some shared library weirdness. Can you apply that on top of your CL?

I'll try to take a closer look at the actual changes on monday, but the changes seem mostly straight-forward.

I do have one request though: Could we make the default value for the test build folder a subfolder of the current directory? Right now if you run dotest from a directory containing some interesting stuff, it will pollute it with a bunch of subfolders and it can be hard to clean it up afterwards. If it was a subfolder with a recognisable name, I could just nuke it and be done with it.

aprantl updated this revision to Diff 131657.Jan 26 2018, 2:19 PM
  • incorporate pavel's Linux patch
  • default the builddir to $PWD/lldb-test-build
aprantl updated this revision to Diff 131658.Jan 26 2018, 2:29 PM
aprantl marked 4 inline comments as done.

Address Jim's comments.

aprantl retitled this revision from WIP: compile the LLDB tests out-of-tree to Compile the LLDB tests out-of-tree.Jan 26 2018, 2:29 PM
aprantl added inline comments.
packages/Python/lldbsuite/test/api/check_public_api_headers/TestPublicAPIHeaders.py
46 ↗(On Diff #131525)

Because this test uses a generated source file, so it's in the build directory. I added a comment.

packages/Python/lldbsuite/test/dotest.py
1196 ↗(On Diff #130546)

This the top-level build directory. In a cmake build it is expected to exist, but on the command line when running only a single test, you might want to pass a something like /tmp/mytest there.

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_set_restart/TestBreakpointSetRestart.py
35–37 ↗(On Diff #131525)

It is redundant. LLDB finds the file just fine with a relative FileSpec. I removed it.

I am now working on building each test variant (dwarf,dwo,dsym,...) in its own build directory so they can run in parallel and we can get rid of the lockfile. While doing this I discovered that certain tests (e.g., TestPublicAPIHeaders.py, the MI tests) are invoking buildDefault() in the setUp() method. From what I can tell it looks like these tests build only whatever variant the Makefile produces by default, but they still run the tests for each variant. I'm not yet sure how to best fix this.

I found the mechanism I was looking for. Generic testcases set
NO_DEBUG_INFO_TESTCASE = True

I am now working on building each test variant (dwarf,dwo,dsym,...) in its own build directory so they can run in parallel and we can get rid of the lockfile.

Are you planning to merge that into this patch? I am hoping that can be done in a separate pass, once dust settles down from landing this batch.

I think this is in a pretty good shape now, and we should land it soon. The only thing i'd like to wait for is confirmation that this runs on windows (i.e. does not run into any fundamental make limitations on that platform). I'm going to see if I can get around to that today.

packages/Python/lldbsuite/test/functionalities/exec/TestExec.py
47 ↗(On Diff #131658)

s/Sourc/Source/

packages/Python/lldbsuite/test/plugins/builder_base.py
65 ↗(On Diff #131658)

delete

source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
5139–5142 ↗(On Diff #131658)

I think it makes sense to separate the single change in actual code from the gigantuous code refactor.

I haven't gotten around to trying this out on windows yet, but I have tried running the tests remotely. I've updated D42572 with the two fixes necessary to make the remote tests pass (for android).

I am now working on building each test variant (dwarf,dwo,dsym,...) in its own build directory so they can run in parallel and we can get rid of the lockfile.

Are you planning to merge that into this patch? I am hoping that can be done in a separate pass, once dust settles down from landing this batch.

I think this is in a pretty good shape now, and we should land it soon. The only thing i'd like to wait for is confirmation that this runs on windows (i.e. does not run into any fundamental make limitations on that platform). I'm going to see if I can get around to that today.

Sure that makes total sense. "Smaller" patches are always better. I'm mostly waiting for someone to greenlight Windows now.

source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
5139–5142 ↗(On Diff #131658)

Oh right. This is an actual bug that I found because my refactoring of makefiles happened to change the relative order of dylibs in the Mach-O headers.

aprantl updated this revision to Diff 131899.Jan 29 2018, 4:22 PM
aprantl marked 3 inline comments as done.

I think I now have addressed everybody's review comments.

labath accepted this revision.Jan 30 2018, 6:57 AM

Right, so I tried this out on windows today (targetting android, because I know the state of that target, but I think this should catch problems with windows host builds as well). The main problem i ran into is that $(realpath) is just broken on windows -- in case the input path is already an absolute path it will just produce nonsensical values. I guess it worked before because MAKEFILE_LIST was relative, but with the new invocation, the variable contains absolute paths. However, I think this means we don't have to call realpath, so I suggest we just drop it.

The other problem was lack of mkdir -p functionality. Even though I have a gnuwin32 mkdir on my PATH (which supports the -p switch), make will execute the command through cmd.exe, which means the built-in mkdir will fire first (and blow up). The simplest solution for this I could come up with is to offload the mkdir functionality to python. I'm not sure I've caught all of the issues, but I think the rest of them can be handled incrementally.

I've uploaded the changes I made to D42572 (it also contains my previous batch of remote testing fixes). In case you agree with them, I say "let's ship it" before the patch starts rotting.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 30 2018, 10:31 AM
This revision was automatically updated to reflect the committed changes.

Let's see how this fares on the bots. As I mentioned in the commit message, please don't hesitate to revert this patch. I'm watching all bots connected to lab.llvm.org, and green.lab.llvm.org but please let me know if I'm missing something.

There are a few test failures, but they look manageable. I will need help resolving them though:

http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/7843
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_dwarf

http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-android/builds/10506
test7 emulator-5554,i686-linux-android-clang,i386,abstract,-lldb-mi failed

There are a few test failures, but they look manageable. I will need help resolving them though:

Updated list:

http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/7843
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_dwarf
test3 HT4A1JT02945,aarch64-linux-android-clang,aarch64,tcp,-lldb-mi 8 unexpected failures failed ( 41 mins, 48 secs )
stdio
FAIL: TestBreakpointInGlobalConstructor.TestBreakpointInGlobalConstructors.test
FAIL: TestMoveNearest.TestMoveNearest.test
FAIL: TestGlobalVariables.GlobalVariablesTestCase.test_c_global_variables_dwarf
FAIL: TestGlobalVariables.GlobalVariablesTestCase.test_c_global_variables_dwo
FAIL: TestGlobalVariables.GlobalVariablesTestCase.test_c_global_variables_gmodules
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_dwarf
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_dwo
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_gmodules

http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-android/builds/10506
test7 emulator-5554,i686-linux-android-clang,i386,abstract,-lldb-mi failed

http://lab.llvm.org:8011/builders/lldb-x86_64-darwin-13.4/builds/8462
test2 5A24000830,aarch64-linux-android-gcc,aarch64,tcp,-lldb-mi:-watchpoint 2 unexpected failures failed
FAIL: TestMoveNearest.TestMoveNearest.test
FAIL: TestBreakpointInGlobalConstructor.TestBreakpointInGlobalConstructors.test

There are a few test failures, but they look manageable. I will need help resolving them though:

Updated list:

http://lab.llvm.org:8011/builders/lldb-windows7-android/builds/7843
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_dwarf
test3 HT4A1JT02945,aarch64-linux-android-clang,aarch64,tcp,-lldb-mi 8 unexpected failures failed ( 41 mins, 48 secs )
stdio
FAIL: TestBreakpointInGlobalConstructor.TestBreakpointInGlobalConstructors.test
FAIL: TestMoveNearest.TestMoveNearest.test
FAIL: TestGlobalVariables.GlobalVariablesTestCase.test_c_global_variables_dwarf
FAIL: TestGlobalVariables.GlobalVariablesTestCase.test_c_global_variables_dwo
FAIL: TestGlobalVariables.GlobalVariablesTestCase.test_c_global_variables_gmodules
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_dwarf
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_dwo
FAIL: TestLoadUnload.LoadUnloadTestCase.test_lldb_process_load_and_unload_commands_gmodules

http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-android/builds/10506
test7 emulator-5554,i686-linux-android-clang,i386,abstract,-lldb-mi failed

http://lab.llvm.org:8011/builders/lldb-x86_64-darwin-13.4/builds/8462
test2 5A24000830,aarch64-linux-android-gcc,aarch64,tcp,-lldb-mi:-watchpoint 2 unexpected failures failed
FAIL: TestMoveNearest.TestMoveNearest.test
FAIL: TestBreakpointInGlobalConstructor.TestBreakpointInGlobalConstructors.test

Most of these turned out to be caused by leftover shared libraries in the source tree (fun fact: "svn status" refuses to list the .so files even though they are not present in the svn:ignore list), when I cleaned those up, the tests started passing again.

There is still the TestLoadUnload failure on the windows bot, which I suspect to be a real issue (we've changed the test a bit, and now it passes full paths to lldb, so I suspect a directory-separator issue), but I have not looked into that yet. I'll handle that on our side in the coming days.

All in all, I would say this was rather a painless experience. I think we are ready for phase two.

Thanks a lot for your help, Pavel!

We also discovered the most amazing test failure on the green dragon bots yesterday (fix still underway) that was made much worse by my patch.
On Darwin LLDB automatically tries to find a .dSYM bundle for an executable by querying the Spotlight index for the executable's UUID. Because the UUID is a hash of filename and .text section, and the testsuite compiles many short hello-world-style files, all alike, and named a.out, two tests can have the same UUID and LLDB then sometimes picks up debug info from a .dSYM that belongs to different testcase! We are working on a mechanism to disable the UUID lookup inside the testsuite.