[Lldb-commits] [PATCH] D30520: Make LLDB skip server-client roundtrip for signals that don't require any actions

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Note that ProcessGDBRemote (or maybe the client) keeps a history array of packets sent.  Greg added that long ago so that when you enabled the packet log you would get the packet history up to the time you enabled it, and not just the new packets.  That has often come  in quite handy.  And having a SB API like:

bool SBProcess::HasRemoteTrafficLog();
SBStringList SBProcess::DumpRemoteTraffic(<some kind of checkpoint>);

doesn't seem like a bad API.  I could imagine some really horribly geeky debugger UI that would want to have a packet traffic window.  Then using the checkpoint, you could get the strings from event to event and look for the packets you expected.  That seems a lot of work for testing this bit of code, however.

Short of that, Pavel's last suggestion seems right for testing that the  contents of the Process's m_unix_signals_sp is correctly copied into the ignore signals packet you are going to send.  The higher level tests already ensure that we are correctly filling m_unix_signals_sp from commands or SB API's, but this last step is uncovered.  So if you move the API to the communication client, and pass in the signals, you can then test that last step in isolation.  And it seems perfectly reasonable for the Communication client to be doing the actual work of copying the signals over.

Another bit that isn't tested is that if you change the signals in the process, you will trigger resending the packet.  You could test more of this path if:

(a) you had the UnixSignals class be the one that returns the array of signals to ignore rather than it being freestanding logic in the ProcessGDBRemote class; the bit of logic that checks pass, stop & notify seems more appropriate in the signals class anyway.

(b) change the m_last_signals_version logic to a "HasChanged" and "SetHasChanged(bool changed)" API vended by UnixSignals. SetHasChanged would be called passing false in the Process::SendSignalsToIgnoreIfNeeded, that seems pretty clearly right.  And you probably have to call SetHasChanged(true) to force things for the launch case, but that should be straightforward.

Then you could test all most of the chain by making your own UnixSignals and passing that to the GDBRemoteCommunicationClient.  If you change it and pass it in again, you could ensure that the right packet is sent.  That leave out that Process handles the SetHasChanged properly but it gets pretty close.


https://reviews.llvm.org/D30520