Page MenuHomePhabricator

Make LLDB skip server-client roundtrip for signals that don't require any actions
ClosedPublic

Authored by eugene on Mar 1 2017, 6:07 PM.

Details

Summary

If QPassSignals packaet is supported by lldb-server, lldb-client will utilize it and ask the server to ignore signals that don't require stops or notifications.
Such signals will be immediately re-injected into inferior to continue normal execution.

Diff Detail

Repository
rL LLVM

Event Timeline

eugene created this revision.Mar 1 2017, 6:07 PM
jingham added a subscriber: jingham.Mar 1 2017, 7:01 PM

The ability to send a set of signals for the remote stub to ignore isn't inherently specific to the GDB Remote protocol. Some other remote protocol could very well implement the same functionality. SendSignalsToIgnoreIfNeeded seems like it should be in Process, and called in the Resume before DoResume gets called and in DidLaunch.

Of course, the default implementation would do nothing, and you would override it with your GDB remote protocol specific version.

Also, it would be good to add a test that ensures that calling "process handle" actually passes the right thing to your SendSignalsToIgnore.

labath edited edge metadata.Mar 2 2017, 3:23 AM

Jim's comment about putting the function in the parent class makes sense -- apart from remote stubs I guess you could also envision other ways a process class could speed up signal processing in case it know we don't care about them).

Jim: do you have a idea about how to write a test for this? These performance-improvement kind of changes don't fit really well into the SB API test framework, as this change should not have any observable behavior, and we already have tests that make sure that "process handle" does the right thing (i.e., we don't stop for ignored signals). The best thing I can come up with (without going on a major refactoring effort) is enabling logging and then grepping it for some string.

Actually, I can think of a potential improvement. Move the code currently in ProcessGDBRemote::SendSignalsToIgnoreIfNeeded to GDBRemoteCommunicationClient. The latter is unit-testable so it would give as more coverage, and I think an API like Error GDBRemoteCommunicationClient::IgnoreSignalsIfSupported(const UnixSignals &signals) would still make sense. It won't solve the problem of testing the whole thing end-to-end, but it would certainly increase the test coverage of this change.

Let me know what you think.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
1067 ↗(On Diff #90265)

It looks like you reformatted the whole file instead of just your changes. Random diff makes review harder. If you set it up correctly git clang-format should only format the code around your changes, so I recommend using that.

3795 ↗(On Diff #90265)

No braces.

unittests/Process/gdb-remote/GDBRemoteCommunicationClientTest.cpp
332 ↗(On Diff #90265)

The documentation for QPassSignals https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html states that "Signals are numbered identically to continue packets and stop replies". While it does not say that directly, I would interpret that as "you should always use two digits for the signal number" because that's how stop-reply packets work. It's fine to keep the receiving code lax, but let's be stricter in what we generate.

jingham added a comment.EditedMar 2 2017, 10:21 AM

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 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 packet is sent, and if you don't change it that it is not. That leaves out that Process handles the SetHasChanged properly but it gets pretty close.

eugene updated this revision to Diff 90407.Mar 2 2017, 3:15 PM

Addressing code review commends, and moving a signal filtering method to the base Process class.

eugene marked 3 inline comments as done.Mar 2 2017, 3:16 PM

I added a few comments. It doesn't look like you addressed Pavel's idea for testing more of the code path you're introducing. Do you plan to do that?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
3766–3776 ↗(On Diff #90407)

This code should go into the UnixSignals class. Any other Process plugin that wanted to implement expedited signal handling would also have to do this computation, and we shouldn't duplicate the code.

source/Target/Process.cpp
1629–1630 ↗(On Diff #90407)

For this to work, you are relying on the Constructor of the signals class to set up the object by calling AddSignal on all the signals. Otherwise initially m_version in the signals class and m_last_signal_version will start out at 0 and you won't prime the filtering here.

You should probably add a comment to that effect somewhere in UnixSignals so we don't forget this dependency.

eugene updated this revision to Diff 90424.Mar 2 2017, 6:46 PM
eugene marked an inline comment as done.

Added comment to UnixSignals::m_version.
Still thinking about the best way to test it all.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
3766–3776 ↗(On Diff #90407)

I think it would be a mistake to over-engineer it and try to anticipate needs of any possible implementation at this point.
Maybe another implementation would require a list of signals that need to be stopped instead of ignored.

Whenever we have an alternative implementation that needs to do such thins we can always go back and generalize this code, as for now I think we need to keep things simple.

I like the concise interface that UnixSignals provides and I don't want to pollute it with things can be easily done in the code that uses it.

source/Target/Process.cpp
1629–1630 ↗(On Diff #90407)

I added a comment to UnixSignals::m_version

labath added a comment.Mar 3 2017, 7:16 AM
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
3766–3776 ↗(On Diff #90407)

I agree with Eugene. I think there are advantages to the UnixSignals class being "dumb", and having the process plugin decide what to do with the data there. If anything, what this code needs is a better signal iteration API, so you could write this as something like:

for (auto signal: signals) {
  if (!signal.stop && !signal.notify && !signal.supress)
    my_list.push_back(signal.number);
}

That still seems wrong to me. The UnixSignals class manages the data about signal state including how to respond to them, so it should be the class that comprehends that data. The mental exercise of "if I had to do this in another place, would I have to duplicate an algorithm wholly specific to this class" is not over-engineering, it's how you decide where responsibility lies. And in this case it's clear that if another process class wanted to do this job, it would have to duplicate this code, which means it doesn't belong in Process.

eugene updated this revision to Diff 90563.Mar 3 2017, 7:52 PM

Moved signal filtering to UnixSignals and added unit tests for UnixSignals class.

labath added a comment.Mar 6 2017, 7:57 AM

Looks great, just a couple of style nits in the tests.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
82 ↗(On Diff #90563)

Is this include still necessary?

unittests/Signals/UnixSignalsTest.cpp
43 ↗(On Diff #90563)

This (and the function) should probably have an EXPECT_.. prefix instead, as it does not abort the evaluation of the function it is in (like other ASSERT_*** macros).

64 ↗(On Diff #90563)

We generally put the "expected" value first, and the observed second. The ASSERT_ macro does that, but here you seem to have inverted it.

That looks good. This is a good addition for processes that are really chatty with some signals. Thanks for working on this.

clayborg requested changes to this revision.Mar 6 2017, 2:34 PM

Pretty close. My only objection is we have many "lldb_private::Process::Will" and "lldb_private::Process::Did" prefixed functions and none of them are required to call the superclass version. I would prefer that this doesn't change. See my inlined comments,

source/Target/Process.cpp
1624–1627 ↗(On Diff #90563)

I would prefer to not require people to call the base class functions for any "Process::Will*" or "Process::Did*" there are many of these and the ideas are that these are things that can be overridden. These aren't called in that many places, so it will be easy to just call this function before calling it, or adding a Process::PrivateWillResume that calls UpdateAutomaticSignalFiltering() followed by Process::WillResume().

1629 ↗(On Diff #90563)

Ditto.

unittests/Signals/UnixSignalsTest.cpp
64 ↗(On Diff #90563)

Is this "expected" value being first documented somewhere? If not it should be. I have seen this comment a few times and it would be great if the headerdoc already says this.

This revision now requires changes to proceed.Mar 6 2017, 2:34 PM

My main objection is that if we have 10 "lldb_private::Process::Will*" functions and only some require you to call the superclass, then it is confusing. It is also hard to enforce. We probably have other process subclasses that override these functions and they all would be broken. It also makes it harder when merging code to other branches that might have an extra process subclass. The merge would go fine, but any process subclasses that exist only in other branches would now be out of date and doing the wrong thing by not calling the superclass.

eugene updated this revision to Diff 90755.Mar 6 2017, 3:47 PM
eugene edited edge metadata.

Addressing review comments on SignalTests and getting rid of dependency on DidLaunch and WillResume

eugene marked 3 inline comments as done.Mar 6 2017, 3:50 PM

Addressing review comments.

unittests/Signals/UnixSignalsTest.cpp
43 ↗(On Diff #90563)

This function calls ASSERT_EQ so it does abort evaluation.

clayborg requested changes to this revision.Mar 6 2017, 4:10 PM

Very close. Can we try to get UpdateAutomaticSignalFiltering out of lldb_private::Process as my inline comments suggest? It would be cleaner and I am not sure we actually need Process::UpdateAutomaticSignalFiltering() for all processes.

include/lldb/Target/Process.h
3148 ↗(On Diff #90755)

Can we remove this and only have it in ProcessGDBRemote? Then we just call it when we need to in ProcessGDBRemote? This seems like something that ProcessGDBRemote should take care of without having Process knowing about it. Seems like a adding a call to ProcessGDBRemote::UpdateAutomaticSignalFiltering() can be done in ProcessGDBRemote::WillResume() and ProcessGDBRemote::DidLaunch() might do the trick? It seems like each process plug-in will have different needs regarding signals.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
401 ↗(On Diff #90755)

Remove the override if we handle this in UpdateAutomaticSignalFiltering in specific to ProcessGDBRemote only?

This revision now requires changes to proceed.Mar 6 2017, 4:10 PM
eugene added inline comments.Mar 6 2017, 4:32 PM
include/lldb/Target/Process.h
3148 ↗(On Diff #90755)

I 100% agree with you here. UpdateAutomaticSignalFiltering() is very much implementation specific and it shouldn't be in the lldb_private::Process. And it was how I originally implemented it in older revision.

But Jim felt differently and I trusted his judgement given that he has vastly more experience working with LLDB codebase.

Jim Ingham said (in email):

I disagree. The different processes are at this point more about transport than about the platform details. That indicates to me that it's more likely that different process implementations will have different ways of implementing signal filtering, but the input of which signals will be filtered will be the same. Plus for each process the place where you would implement turning on and changing the filtering are the same. So it makes more sense to have the part that says "go do this filtering" all be in common code, and the particular plugins implement how to do this.

That is fine. Seems like the UnixSignals class should be coordinating this instead of doing it manually in the process? Seems like the UnixSignals class should have guts here and call through to the Process::UpdateAutomaticSignalFiltering() only if needed?

eugene added a comment.Mar 6 2017, 5:09 PM

UnixSignals is a nice self contained class that already does 99% of the work (see UnixSignals::GetFilteredSignals). I don't think we should have it call anybody.
Only process knows when it is the right time to send actual QPassSignals packet, there is not need to somehow push this knowledge into UnixSignals.

Let's just decide if UpdateAutomaticSignalFiltering() is specific enough to live in GDBProcess or it's generic enough to live in the base process. I'm fine either way.

clayborg accepted this revision.Mar 6 2017, 5:24 PM

Ok, sounds like everyone thought through the solution. Lets start with this and we can iterate if needed.

This revision is now accepted and ready to land.Mar 6 2017, 5:24 PM
labath added a comment.Mar 7 2017, 3:05 AM
unittests/Signals/UnixSignalsTest.cpp
43 ↗(On Diff #90563)

Unfortunately it does not. If you look up what ASSERT_EQ does, you'll see that it basically amounts to EXPECT_EQ(a,b); return; https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#assertion-placement, https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#using-assertions-in-sub-routines. So it does abort, but only the top most frame. Since your topmost frame is the AssertEqArrays function, it will just do an early return from that, and the caller will happily continue. If you wanted to be fancy you could define the macro to something like CompareArrays(...); if(HasFailure()) return;. Another solution would be to write a comparison function that returns an AssertionResult https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#using-a-function-that-returns-an-assertionresult and then use that in EXPECT_TRUE/ASSERT_TRUE as you see fit (but that's probably overkill).

eugene updated this revision to Diff 90921.Mar 7 2017, 12:56 PM

Rename ASSERT_EQ_ARRAYS to EXPECT_EQ_ARRAYS

eugene marked an inline comment as done.Mar 7 2017, 12:56 PM
labath accepted this revision.Mar 7 2017, 1:34 PM

Thank you.

This revision was automatically updated to reflect the committed changes.