This is an archive of the discontinued LLVM Phabricator instance.

[Reproducers] Add a tool to transparently capture and replay lldb sessions
ClosedPublic

Authored by JDevlieghere on Jan 15 2020, 8:48 PM.

Details

Summary

This patch introduces a small new utility (lldb-repro) to transparently capture and replay debugger sessions from the driver. This makes it possible to test the reproducers by running the test suite twice, once to capture a reproducer for every lldb invocation, and then running the test suite again that replays the previously recorded session.

$ ./bin/llvm-lit ../llvm-project/lldb/test/Shell/ -sv --param=lldb-run-with-repro=capture
$ ./bin/llvm-lit ../llvm-project/lldb/test/Shell/ -sv --param=lldb-run-with-repro=replay

Diff Detail

Event Timeline

JDevlieghere created this revision.Jan 15 2020, 8:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 8:48 PM
Herald added a subscriber: mgorny. · View Herald Transcript

This seems like it could be useful. Some random questions inline..

lldb/test/Shell/lit.cfg.py
42–57

The difference between these two is super-unclear. Any chance to unify them?

lldb/tools/lldb-repro/lldb-repro.cpp
23 ↗(On Diff #238413)

Is the r in the name intentional?

63 ↗(On Diff #238413)

I am wondering if this shouldn't even be the default behavior. If I already passed --capture to lldb then it does not seem unreasonable to always write it out when lldb exits (we already do it after a crash, right?)

lldb/tools/lldb-repro/lldb-repro.h.cmake
12 ↗(On Diff #238413)

Are you sure this will work fine with multi-config generators? You might be better off just relying on the fact that the lldb executable will sit right next to this binary...

Considered making this a python script?

lldb/tools/lldb-repro/lldb-repro.h.cmake
12 ↗(On Diff #238413)

Actually how, is this thing going to be invoked exactly? Couldn't the path to lldb be passed simply as argv[1]?

JDevlieghere marked 4 inline comments as done.Jan 16 2020, 9:07 AM

Considered making this a python script?

I think that makes sense

lldb/test/Shell/lit.cfg.py
42–57

Not really, one is for the tool, the other is to enable reproducer capture by default. They're totally orthogonal, but I agree it's confusing.

lldb/tools/lldb-repro/lldb-repro.cpp
23 ↗(On Diff #238413)

Nope, this is a typo

63 ↗(On Diff #238413)

My ultimate goal is to have reproducers enabled by default. The capture flag is just away to make that behavior opt-in for now. You might use it to get a reproducer on a crash, but you don't necessary want to keep all reproducers around otherwise.

lldb/tools/lldb-repro/lldb-repro.h.cmake
12 ↗(On Diff #238413)

It just needs patching up like lldb-dotest and lit. Assuming you mean argv[0], it think we could make that work if I replace "%lldb" with a path to lldb-repro.

aprantl added inline comments.
lldb/test/Shell/lit.cfg.py
42–57

How about adding this as a comment here?

lldb/tools/lldb-repro/lldb-repro.cpp
25 ↗(On Diff #238413)

hash_combine()?

lldb/tools/lldb-repro/lldb-repro.h.cmake
5 ↗(On Diff #238413)

Out of curiosity, is the Copyright 2020 line part of the recommended default file header?

aprantl added inline comments.Jan 16 2020, 9:47 AM
lldb/tools/lldb-repro/lldb-repro.h.cmake
5 ↗(On Diff #238413)

To answer my own question: https://www.llvm.org/docs/CodingStandards.html#file-headers

No, that's a Swift-lldb-ism. That's great because those lines are annoying to keep up to date anyway.

  • Convert to Python
  • Integrate with lit
JDevlieghere edited the summary of this revision. (Show Details)Jan 16 2020, 11:33 AM
aprantl added inline comments.Jan 16 2020, 12:52 PM
lldb/utils/lldb-repro/lldb-repro.py
2

Could we add a top-level comment (and/or a Usage: message) that explains what this tool is used for? That's not really obvious for someone who hasn't read the commit message.

Add a comment explaining the purpose of the utility.

JDevlieghere marked an inline comment as done.Jan 16 2020, 1:21 PM
aprantl added inline comments.Jan 16 2020, 2:43 PM
lldb/utils/lldb-repro/lldb-repro.py
50

Thanks! Sorry for being such a nuisance, but I think this should be a doc-comment (""") at the top of the file?

JDevlieghere marked an inline comment as done.
JDevlieghere marked an inline comment as done.Jan 16 2020, 3:05 PM
JDevlieghere added inline comments.
lldb/utils/lldb-repro/lldb-repro.py
50

No worries, totally fair!

aprantl accepted this revision.Jan 16 2020, 3:29 PM
This revision is now accepted and ready to land.Jan 16 2020, 3:29 PM
labath added inline comments.Jan 17 2020, 1:20 AM
lldb/test/Shell/helper/toolchain.py
13

Maybe _get_lldb_init_path? This way it looks like this function is doing some fancy initialization..

lldb/test/Shell/lit.cfg.py
42–57

Yes, that's better now, though, per the other comment, I think the env vars can be removed completely.

lldb/tools/lldb-repro/lldb-repro.cpp
63 ↗(On Diff #238413)

Ok, that makes sense. That said, I think we will always want a way to disable this -- this just looks too much like one of those "this shit is tracking everything I do" features a lot of people will want to stay away from.

lldb/tools/lldb-repro/lldb-repro.h.cmake
12 ↗(On Diff #238413)

No, I really meant argv[1]. :)

The idea was that %lldb would expand to /src/path/to/lldb-repro.py /build/path/to/lldb.exe --whatever. That way, you wouldn't need to rely on the "same directory" trick and could get rid of all the cmake code. In fact, we could even throw in a --capture/--replay argument to the command line, and ditch the environment variables too...

JDevlieghere marked 11 inline comments as done.
  • Remove environment variables
JDevlieghere marked an inline comment as done.Jan 17 2020, 12:35 PM
JDevlieghere added inline comments.
lldb/tools/lldb-repro/lldb-repro.h.cmake
12 ↗(On Diff #238413)

I like the idea but FindTool is a class that's resolved by lit, and the arguments are strings. So I kept the current approach that expects to find lldb next to lldb-repro.

labath accepted this revision.Jan 20 2020, 12:07 AM
labath added inline comments.
lldb/tools/lldb-repro/lldb-repro.h.cmake
12 ↗(On Diff #238413)

Ah, yes, that does make things tricky.. Yeah, having both things in the same directory seems fine...

lldb/utils/lldb-repro/lldb-repro.py
25

I guess you don't need the /path/to/lldb part then?

JDevlieghere marked 2 inline comments as done.Jan 20 2020, 10:26 AM
JDevlieghere added inline comments.
lldb/utils/lldb-repro/lldb-repro.py
25

Right, forgot to undo that change again. Thx!

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.