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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
