This is an archive of the discontinued LLVM Phabricator instance.

[Reproducer] Move GDB Remote Packet into Utility. (NFC)
ClosedPublic

Authored by JDevlieghere on Sep 12 2019, 2:46 PM.

Details

Summary

To support dumping the reproducer's GDB remote packets, we need the
(de)serialization logic to live in Utility rather than the GDB remote plugin.
This patch renames StreamGDBRemote to GDBRemote and moves the relevant packet
code there. Its uses in the GDBRemoteCommunicationHistory and the
GDBRemoteCommunicationReplayServer are updated as well.

Diff Detail

Event Timeline

JDevlieghere created this revision.Sep 12 2019, 2:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 2:46 PM
davide accepted this revision.Sep 12 2019, 2:48 PM
davide added a subscriber: davide.

Easy peasy.

This revision is now accepted and ready to land.Sep 12 2019, 2:48 PM

It seems weird to me to have the GDBRemotePacket class live in the repro namespace. There's nothing particularly Reproducer specific about it, and it clearly gets used outside the reproducer as well. Wouldn't it make more sense to have this in Utils?

It seems weird to me to have the GDBRemotePacket class live in the repro namespace. There's nothing particularly Reproducer specific about it, and it clearly gets used outside the reproducer as well. Wouldn't it make more sense to have this in Utils?

Yep, I agree, and I considered alternatives but couldn't find a better solution. It's seems too specific for a utility class in itself and doesn't belong in Core or Target either. It really should be part of the GDB plugin, but then we can't access it without layering violations from Command...

There's already Utils/StreamGDBRemote.cpp... It might be natural to stick the class in there?

JDevlieghere planned changes to this revision.Sep 12 2019, 3:33 PM
JDevlieghere retitled this revision from [Reproducer] Move GDB packet struct into the reproducer. (NFC) to [Reproducer] Move GDB Remote Packet into Utility. (NFC).
JDevlieghere edited the summary of this revision. (Show Details)

Move code into GDBRemote

This revision is now accepted and ready to land.Sep 12 2019, 3:47 PM
jingham accepted this revision.Sep 13 2019, 4:02 PM

Looks right.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 4:18 PM
labath added a subscriber: labath.Sep 16 2019, 12:30 AM

*Maybe* this is fine for now, but I also don't like moving the gdb-remote reproducer stuff out of the gdb plugin. If you want an inspection command to access those (which sounds like a useful thing btw), then the right way to handle that would be to have some sort of an abstraction/plugin mechanism for various reproducers. For instance, the reproducers could register themselves somewhere (if they don't do that already), so we get a list of all of them, similar to how we handle "plugins" now. Then the inspection command could iterate over that list and ask each reproducer component to do its thing. This may be clunky, or it may not, depending on what exactly this "thing" is.