[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 added inline comments.


================
Comment at: lldb/include/lldb/Target/Target.h:1451
+  /// Print all the signals set in this target.
+  void PrintDummySignals(Stream &strm, Args signals, size_t num_signals);
+
----------------
JDevlieghere wrote:
> Was `Args` supposed to be a reference here? If not why do we need the copy?
> 
> I didn't look at the implementation yet, but why do we need `num_signals`? Can't we count the number of args?
Args should be a reference, that was an oversight.  Thanks for catching that.

The num_signals was to make the function parallel to the one that printed the process symbols, but wasn't necessary for the dummy signals, so I removed it.


================
Comment at: lldb/include/lldb/Target/Target.h:1533
   lldb::StackFrameRecognizerManagerUP m_frame_recognizer_manager_up;
+  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 
----------------
JDevlieghere wrote:
> Does it matter that these are ordered? Can this use a `llvm::StringMap`?
This is a map that's going to have a most a small handful of elements and is not on a critical path for access/copying/etc, so I don't think the actual container matters much.  StringMap is marginally nicer when we set the value, so I switched to it.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1477-1479
+    bool only_target_values;
+    bool do_clear;
+    bool dummy;
----------------
JDevlieghere wrote:
> Let's initialize these to the same values as `Clear`.
I generally don't initialize the Option ivars on construction, on the grounds that it will mislead people into thinking the initialized values actually matter, which they don't.  They are never used nor should they be.  You always have to call OptionParsingStarting before reading in values for the Options, and you have to reset all the variables there.

But if it bugs you, I can add it.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1582
   bool DoExecute(Args &signal_args, CommandReturnObject &result) override {
-    Target *target_sp = &GetSelectedTarget();
+    Target *target_sp = &GetSelectedOrDummyTarget();
 
----------------
JDevlieghere wrote:
> Not your change but why `Target *` and not `Target &`?
I don't know.  That code is confused anyway, because GetSelectedTarget returns a `Target &`, but the variable is called target_sp meaning at some point in the past this must have been a TargetSP?

Anyway, CommandObject::GetSelectedOrDummyTarget always returns a valid target, so I changed this to `Target &` and took off the spurious `_sp`


================
Comment at: lldb/source/Target/Target.cpp:3357
 
+void Target::AddDummySignal(const char *name, LazyBool pass, LazyBool notify, 
+                            LazyBool stop) {
----------------
JDevlieghere wrote:
> It seems like this can take a `std::string`and `std::move` it into the pair below. Or a `StringRef` if you turn this into a StringMap as per the other suggestion.
I just changed the API to take a StringRef.


================
Comment at: lldb/source/Target/Target.cpp:3366-3374
+    auto elem = m_dummy_signals.find(name);
+    if (elem != m_dummy_signals.end()) {
+      (*elem).second.pass = pass;
+      (*elem).second.notify = notify;
+      (*elem).second.stop = stop;
+      return;
+    }
----------------
JDevlieghere wrote:
> With a StringMap you can simplify all of this into:
> 
> ```
> auto& entry = m_dummy_signals[name];
> entry.pass = pass;
> entry.notify = notify;
> ...
> ```
Nice.


================
Comment at: lldb/source/Target/Target.cpp:3386-3389
+  if (elem.second.pass == eLazyBoolYes)
+    signals_sp->SetShouldSuppress(signo, false);
+  else if (elem.second.pass == eLazyBoolNo)
+    signals_sp->SetShouldSuppress(signo, true);
----------------
JDevlieghere wrote:
> I'm wondering if we can simplify this with a little utility function. Something like this:
> 
> ```
> static void helper(LazyBool b, std::function<void(bool)> f) {
>   switch(b) {
>     case eLazyBoolYes:
>       return f(true);
>     case eLazyBookFalse:
>       return f(false);
>     case eLazyBoolCalculate:
>       return;
>   }
>   llvm_unreachable("all cases handled");
> }
> ```
> 
> That way we can simplify this:
> 
> ```
> helper(elem.second.pass, [](bool b) { signals_sp->SetShouldSuppress(signo, b); });
> ```
If you don't mind, I'd rather not do that.  

The utility function is a little too narrow to be generally useful, since it only works on functions that take one bool and return void.  And if we add the utility function here, then between it and the usages, the code I have already is IMO a lot easier to read.


================
Comment at: lldb/source/Target/Target.cpp:3436-3438
+  UnixSignalsSP signals_sp;
+  if (process_sp)
+    process_sp->GetUnixSignals();
----------------
JDevlieghere wrote:
> Should this have been 
> 
> ```
> if (process_sp)
>     signals_sp = process_sp->GetUnixSignals();
> ```
> 
> Now `signals_sp` is never initialized. 
Yup.  That was a "prettifying the patch for it's diff" mis-change.  It caused the test to crash, so I just made it right again.


================
Comment at: lldb/source/Target/Target.cpp:3458-3464
+    auto str_for_lazy = [] (LazyBool lazy) -> const char * {
+      switch (lazy) {
+        case eLazyBoolCalculate: return "not set";
+        case eLazyBoolYes: return "true   ";
+        case eLazyBoolNo: return "false  ";
+      }
+    };
----------------
JDevlieghere wrote:
> This seems super useful. Maybe this function, together with the other utility I suggested, can go in a LazyBoolUtils or something in Utility. 
This utility asserts a meaning for the three values which might or might not be appropriate in other circumstances.  Given it's so simple to write, I think it's better to let the use site choose what the appropriate text is for the three value than appear to be mandating it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259