jingham marked 3 inline comments as done.
jingham added a comment.
This patch treats the Signal structure held in the target differently from
UnixSignals::Signal. The latter always has to have values for all three
actions and needs to have a signal number. The former doesn't actually claim
to know a signal number, it only works on strings, since it might get set
before we can know what signal number a given signal string has (signals with
the same name do have different numbers on different platforms). Also, it
doesn't record actual values, it records user intents, so it needs to be
tri-state - thus the LazyBools.
I think the two are sufficiently different in use that it's cleaner to have
separate types for them.
================
Comment at: lldb/include/lldb/Target/Target.h:1421
+protected:
+ struct DummySignalValues {
+ LazyBool pass = eLazyBoolCalculate;
----------------
clayborg wrote:
> We should make UnixSignals::Signal a public class and then just save a
> std::vector of those inside this class. That class doesn't contain a signal
> number, so we should be able to re-use it here and avoid creating a new class
> that mimic what is contains.
The data structure held by the target is different from the one that represents
actual signals because the Target one needs to record not just "true or false"
but "whether set by the user." That's why I used LazyBools not bools in the
structure. That's important because if you are setting a signal handling in
the .lldbinit you don't yet know what the default value is, so you need to be
able to say "change print to false, and leave the others at their default
values".
So I don't think that the UnixSignals::Signal is the right data structure of
rthis.
================
Comment at: lldb/include/lldb/Target/Target.h:1429
+ };
+ using DummySignalElement = std::pair<std::string, DummySignalValues>;
+ static bool UpdateSignalFromDummy(lldb::UnixSignalsSP signals_sp,
----------------
clayborg wrote:
> Maybe store a std::vector<UnixSignal::Signal> objects filled out as much as
> possible here? Then we don't need the DummySignalElement type at all.
See the comment above.
================
Comment at: lldb/include/lldb/Target/Target.h:1533-1536
+ std::map<std::string, DummySignalValues> m_dummy_signals;/// These are used
to
+ /// set the signal state when you don't have a process and more usefully
+ /// in the Dummy target where you can't know exactly what signals you
+ /// will have.
----------------
clayborg wrote:
> Switch to std::vector< UnixSignal::Signal> here and then we can prime the
> UnixSignals with the vector?
See the first comment.
================
Comment at: lldb/include/lldb/Target/UnixSignals.h:120
bool m_suppress : 1, m_stop : 1, m_notify : 1;
+ bool m_default_suppress : 1, m_default_stop : 1, m_default_notify : 1;
----------------
clayborg wrote:
> If we store a vector<UnixSignals::Signal> in the target, can we reset the
> default values from another "Signal" struct instead of saving it here? Not a
> big deal if not, but just wondering.
I wondered about that, but having to make another version of Signals and hope
it's constructed the same way seems a fallible way to get the default value the
Signal object was constructed with. This is just 3 bytes, and won't actually
change the size of the structure (plus these are not structures we have lots
and lots of). So I think doing the straightforward thing is better.
================
Comment at: lldb/include/lldb/Target/UnixSignals.h:126
~Signal() = default;
+ void Reset(bool reset_stop, bool reset_notify, bool reset_suppress);
};
----------------
clayborg wrote:
> To follow up with he above comment, this _could_ become:
> ```
> void Reset(const UnixSignals::Signal &signal);
> ```
> we can find the UnixSignals::Signal object for this signal first in the
> target and pass it down into here?
The code that's resetting this doesn't really need to know what values the
signal currently has. It knows that it changed "pass" but not "notify" or
"suppress" and just wants them reset to the default values. Having to look up
some other Signal to do this seems needlessly complex.
================
Comment at: lldb/source/Commands/Options.td:754
+ def process_handle_dummy : Option<"dummy", "d">, Group<2>,
+ Desc<"Also clear the values in the dummy target so they won't be inherited
by new targets.">;
}
----------------
clayborg wrote:
> Do we need the "Also" in the documentation here? Is this option only
> available when used with another option?
Right now, you can either clear the values in the current Target or in the
Current target and the Dummy Target. So the "also" is correct. If you think
it's useful to only clear the dummy target's values, I can change the code to
do that easily.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126259/new/
https://reviews.llvm.org/D126259
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
