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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/include/lldb/Target/Target.h:1421
+protected:
+  struct DummySignalValues {
+    LazyBool pass = eLazyBoolCalculate;
----------------
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. 


================
Comment at: lldb/include/lldb/Target/Target.h:1429
+  };
+  using DummySignalElement = std::pair<std::string, DummySignalValues>;
+  static bool UpdateSignalFromDummy(lldb::UnixSignalsSP signals_sp, 
----------------
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.


================
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.
----------------
Switch to std::vector< UnixSignal::Signal> here and then we can prime the UnixSignals with the vector?


================
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;
 
----------------
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.


================
Comment at: lldb/include/lldb/Target/UnixSignals.h:126
     ~Signal() = default;
+    void Reset(bool reset_stop, bool reset_notify, bool reset_suppress);
   };
----------------
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?


================
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.">;
 }
----------------
Do we need the "Also" in the documentation here? Is this option only available when used with another option?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126259/new/

https://reviews.llvm.org/D126259