[Lldb-commits] [PATCH] D126259: Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created

[Jim Ingham via Phabricator via lldb-commits]

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