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
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
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
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:
> >
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
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
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().
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
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
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
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
>
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
> 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
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
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),
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
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
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
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
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?
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
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
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
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
+
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
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
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
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
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
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.
30 matches
Mail list logo