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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere 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);
+
----------------
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?


================
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 
----------------
Does it matter that these are ordered? Can this use a `llvm::StringMap`?


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1477-1479
+    bool only_target_values;
+    bool do_clear;
+    bool dummy;
----------------
Let's initialize these to the same values as `Clear`.


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1582
   bool DoExecute(Args &signal_args, CommandReturnObject &result) override {
-    Target *target_sp = &GetSelectedTarget();
+    Target *target_sp = &GetSelectedOrDummyTarget();
 
----------------
Not your change but why `Target *` and not `Target &`?


================
Comment at: lldb/source/Target/Process.cpp:2931-2934
+    if (m_unix_signals_sp) {
+      StreamSP warning_strm = GetTarget().GetDebugger().GetAsyncErrorStream();
+      GetTarget().UpdateSignalsFromDummy(m_unix_signals_sp, warning_strm);
+    }
----------------
Clang format


================
Comment at: lldb/source/Target/Target.cpp:293-294
   m_suppress_stop_hooks = false;
+  Args signal_args;
+  ClearDummySignals(signal_args);
 }
----------------
Instead of having to pass an empty Args here, can we make the input argument an `Optional<Args> = {}`? Then we can just call `ClearDummySignals` here. This looks like `signal_args` is an output argument. 


================
Comment at: lldb/source/Target/Target.cpp:3357
 
+void Target::AddDummySignal(const char *name, LazyBool pass, LazyBool notify, 
+                            LazyBool stop) {
----------------
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.


================
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;
+    }
----------------
With a StringMap you can simplify all of this into:

```
auto& entry = m_dummy_signals[name];
entry.pass = pass;
entry.notify = notify;
...
```


================
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);
----------------
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); });
```


================
Comment at: lldb/source/Target/Target.cpp:3436-3438
+  UnixSignalsSP signals_sp;
+  if (process_sp)
+    process_sp->GetUnixSignals();
----------------
Should this have been 

```
if (process_sp)
    signals_sp = process_sp->GetUnixSignals();
```

Now `signals_sp` is never initialized. 


================
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  ";
+      }
+    };
----------------
This seems super useful. Maybe this function, together with the other utility I suggested, can go in a LazyBoolUtils or something in Utility. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126259