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

This revision was automatically updated to reflect the committed changes. Closed by commit rL297231: Make LLDB skip server-client roundtrip for signals that don't require any… (authored by eugene). Changed prior to commit: https://reviews.llvm.org/D30520?vs=90921=90930#toc Repository: rL

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

labath accepted this revision. labath added a comment. Thank you. https://reviews.llvm.org/D30520 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

eugene updated this revision to Diff 90921. eugene added a comment. Rename ASSERT_EQ_ARRAYS to EXPECT_EQ_ARRAYS https://reviews.llvm.org/D30520 Files: include/lldb/Target/Process.h include/lldb/Target/UnixSignals.h source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

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

labath added a comment. Comment at: unittests/Signals/UnixSignalsTest.cpp:43 + +#define ASSERT_EQ_ARRAYS(expected, observed) \ + AssertEqArrays((expected), (observed), __FILE__, __LINE__); eugene wrote: > labath wrote: > >

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

Yes formally that seems problematic. It wouldn't be a problem in practice because you should only call ResetNeedsUpdating in UpdateAutomaticSignalFiltering, and the only place where UpdateAutomaticSignalFiltering should be called is when Launching or Resuming, and you can't call either of

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

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Ok, sounds like everyone thought through the solution. Lets start with this and we can iterate if needed. https://reviews.llvm.org/D30520

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

UnixSignals::NeedsUpdating() method suggests that there is always at most one observer of changes in UnixSignals, the guys who clear the flag. Even though it might be the case now, it still feels like a bomb waiting to explode when another observer starts calling UnixSignals::ResetNeedsUpdating().

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

I like it in the base class and Greg was okay with that too. So let's leave that where it is. The only bit of this behavior that could be moved into UnixSignals as it seemed to me was the handling of "needs updating". I was mostly proposing that because then you can already pass the

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

eugene added a comment. 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

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

The UnixSignals class produces the array of signal numbers that it knows it doesn't want to hear about. But it has no idea how any particular Process plugin would implement ignoring those symbols. So that part belongs to the Process plugin. I suggested in a previous comment also adding a

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

clayborg added a comment. 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 >

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

eugene added inline comments. Comment at: include/lldb/Target/Process.h:3148 + virtual Error UpdateAutomaticSignalFiltering(); + clayborg wrote: > Can we remove this and only have it in ProcessGDBRemote? Then we just call it > when we need to in

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

> On Mar 6, 2017, at 4:10 PM, Greg Clayton via Phabricator > wrote: > > clayborg requested changes to this revision. > clayborg added a comment. > This revision now requires changes to proceed. > > Very close. Can we try to get UpdateAutomaticSignalFiltering out of

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

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. 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

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

eugene marked 3 inline comments as done. eugene added a comment. Addressing review comments. Comment at: unittests/Signals/UnixSignalsTest.cpp:43 + +#define ASSERT_EQ_ARRAYS(expected, observed) \ + AssertEqArrays((expected), (observed),

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

eugene updated this revision to Diff 90755. eugene added a comment. Addressing review comments on SignalTests and getting rid of dependency on DidLaunch and WillResume https://reviews.llvm.org/D30520 Files: include/lldb/Target/Process.h include/lldb/Target/UnixSignals.h

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

clayborg added a comment. 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

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

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. 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

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

jingham added a comment. That looks good. This is a good addition for processes that are really chatty with some signals. Thanks for working on this. https://reviews.llvm.org/D30520 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

labath added a comment. Looks great, just a couple of style nits in the tests. Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:82 +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringSwitch.h" Is this include still necessary?

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

eugene updated this revision to Diff 90563. eugene added a comment. Herald added a subscriber: mgorny. Moved signal filtering to UnixSignals and added unit tests for UnixSignals class. https://reviews.llvm.org/D30520 Files: include/lldb/Target/Process.h include/lldb/Target/UnixSignals.h

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

jingham added a comment. 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

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

labath added a comment. Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3766-3776 + llvm::SmallVector signals_to_ignore; + int32_t signo = m_unix_signals_sp->GetFirstSignalNumber(); + while (signo != LLDB_INVALID_SIGNAL_NUMBER) { +bool

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

jingham added a comment. 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? Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3766-3776 +

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

eugene updated this revision to Diff 90407. eugene added a comment. Addressing code review commends, and moving a signal filtering method to the base Process class. https://reviews.llvm.org/D30520 Files: include/lldb/Target/Process.h include/lldb/Target/UnixSignals.h

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

labath added a comment. 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

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

jingham added a comment. Also, it would be good to add a test that ensures that calling "process handle" actually passes the right thing to your SendSignalsToIgnore. https://reviews.llvm.org/D30520 ___ lldb-commits mailing list

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

jingham added a comment. Of course, the default implementation would do nothing, and you would override it with your GDB remote protocol specific version. https://reviews.llvm.org/D30520 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

jingham added a comment. 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

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

eugene created this revision. 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.