Page MenuHomePhabricator

Add "lldb-test breakpoint" command and convert the case-sensitivity test to use it
ClosedPublic

Authored by labath on Feb 23 2018, 10:50 AM.

Details

Summary

The command takes two input arguments: a module to use as a debug target
and a file containing a list of commands. The command will execute each
of the breakpoint commands in the file and dump the breakpoint state
after each one.

The commands are expected to be breakpoint set/remove/etc. commands, but
I explicitly allow any lldb command here, so you can do things like
change setting which impact breakpoint resolution, etc. There is also a
"-persistent" flag, which causes lldb-test to *not* automatically clear
the breakpoint list after each command. Right now I don't use it, but
the idea behind it was that it could be used to test more complex
combinations of breakpoint commands (set+modify, set+disable, etc.).

Right now the command prints out only the basic breakpoint state, but
more information can be easily added there. To enable easy matching of
the "at least one breakpoint location found" state, the command
explicitly prints out the string "At least one breakpoint location.".

To enable testing of breakpoints set with an absolute paths, I add the
ability to perform rudimentary substitutions on the commands: right now
the string %p is replaced by the directory which contains the command
file (so, under normal circumstances, this will perform the same
substitution as lit would do for %p).

I use this command to rewrite the TestBreakpointCaseSensitivity test --
the test was checking about a dozen breakpoint commands, but it was
launching a new process for each one, so it took about 90 seconds to
run. The new test takes about 0.3 seconds for me, which is approximately
a 300x speedup.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Feb 23 2018, 10:50 AM

Curious if we need lldb-test for this. Could we get by with just using lldb in batch mode? Obviously I'm not opposed to adding whatever we need to lldb-test, just want to make sure it's something that can't already be done with just lldb.

lit/Breakpoint/case-insensitive.test
1 ↗(On Diff #135666)

OSX has a case insensitive file system too. Should we run this test on OSX as well?

This test has the advantage over running lldb in batch mode that it isolates the tests you write using it from the output format of the break command.

The lldb test suite used to check the output of break all over the place, and that meant when I went to change that output to add some info and many tests broke for no good reason. That's why I changed all the tests to use the lldbutil.run_break_set_* commands so I wouldn't have to deal with this again. Not having to do that for these tests some time in the future to me justifies the effort of supporting this small amount of code.

Curious if we need lldb-test for this. Could we get by with just using lldb in batch mode? Obviously I'm not opposed to adding whatever we need to lldb-test, just want to make sure it's something that can't already be done with just lldb.

There are a couple of things which make this tricky. Neither of them in insurmountable I think, but together they add up:

  • without the %p substitution it would be hard to perform an absolute-path match
  • the "auto-clear" and "auto-dump" behavior is quite handy to avoid repetition in the tests
  • the output format can be tweaked to enable easy FileCheck-ing ("At least one breakpoint location"), and can be independent of the actual format we want to present to the users.
lit/Breakpoint/case-insensitive.test
1 ↗(On Diff #135666)

Yes, but they don't do case-insensitive matches by default. This matches the logic of the original test, which matches the logic of the code inside lldb. If the lldb implementation changes, then the test will need to be updated.

davide accepted this revision.Feb 23 2018, 11:20 AM

I was going to suggest the same thing Zach suggested, but I think this fine as is.
LGTM. The fact the test is more concise is definitely a win, but I don't think this is the main reason for doing the conversion.
The 300x speedup makes this change a no-brainer.

This revision is now accepted and ready to land.Feb 23 2018, 11:20 AM

(and thanks for saving 1 minutes and 30 seconds of my life multiplied by the many times I run the test suite per day).

vsk accepted this revision.Feb 23 2018, 11:46 AM
vsk added a subscriber: vsk.

I like that we can customize the command substitutions and the way we dump internal state in this test mode. This offers some added flexibility over the batch mode, IMO. LGTM, thanks!

This revision was automatically updated to reflect the committed changes.