This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Reproducers] Support multiple GDB remotes
ClosedPublic

Authored by JDevlieghere on Dec 5 2019, 10:25 PM.

Details

Summary

When running the test suite with always capture on, a handful of tests are failing because they have multiple targets and therefore multiple GDB remote connections. The current reproducer infrastructure is capable of dealing with that.

This patch reworks the GDB remote provider to support multiple GDB remote connections, similar to how the reproducers support shadowing multiple command interpreter inputs. The provider now keeps a list of packet recorders which deal with a single GDB remote connection. During replay we rely on the order of creation to match the number of packets to the GDB remote connection.

Diff Detail

Event Timeline

JDevlieghere created this revision.Dec 5 2019, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2019, 10:25 PM
Herald added a subscriber: abidh. · View Herald Transcript

This patch is still missing a test and could use another pass over the PacketRecorder and ProcessGDBRemoteProvider to eliminate code duplication with DataRecorder and the CommandProvider on which they're based respectively. I wasn't able to get around to that today but figured I'd upload the patch anyway to give you a change to look at the approach.

Looks pretty straight-forward, but I am still not happy about how more and more of ProcessGDBRemote is infiltrating its way into the Utility module. I think that the reproducer code specific to gdb-remote should live next to the code it's capturing, and Utility/Reproducer.cpp should just provide the general architecture...

lldb/include/lldb/Utility/Reproducer.h
384

is this used anywhere?

lldb/source/Commands/CommandObjectReproducer.cpp
448–450

static? Is it not possible to dump multiple reproducer files in one session ?

454

maybe while ((gdb_file = multi_loader->GetNextFile())) (with an optional != None) ?

JDevlieghere marked an inline comment as done.
  • Address CR feedback.
  • Introduce AbstractRecorder to reuse code between the PacketRecorder and the DataRecorder.
  • Move GDB related reproducer code into Utility/GDBRemote.h.
  • Rename ProcessGDBRemoteProvider to GDBRemoteProvider.
labath added a comment.Dec 9 2019, 4:32 AM

This looks better. Ideally I wouldn't even have Utility/GDBRemote.h, but as Process/gdb-remote is not a very good place for it either (it messes with lldb-server dependencies), we can live with it for now.

lldb/include/lldb/Utility/Reproducer.h
23

I guess this should not be needed anymore?

lldb/source/Commands/CommandObjectReproducer.cpp
448–450

So what about this static thing?

JDevlieghere marked 2 inline comments as done.
JDevlieghere added inline comments.
lldb/source/Commands/CommandObjectReproducer.cpp
448–450

This is totally bogus, I don't even remember why or how it ended up there.

JDevlieghere marked 3 inline comments as done.Dec 9 2019, 8:26 AM

This looks better. Ideally I wouldn't even have Utility/GDBRemote.h, but as Process/gdb-remote is not a very good place for it either (it messes with lldb-server dependencies), we can live with it for now.

Yeah, the other problem is the command object which needs to be able to deserialize the packets to dump them. Layering is hard :(

labath accepted this revision.Dec 10 2019, 9:56 AM
This revision is now accepted and ready to land.Dec 10 2019, 9:56 AM
This revision was automatically updated to reflect the committed changes.