Page MenuHomePhabricator

Add the ability to specify signal actions (pass, stop or notify) for a signal before a process is created
ClosedPublic

Authored by jingham on May 23 2022, 5:21 PM.

Details

Summary

One of the weaknesses of the signal handling model in lldb is that we access the information about signals from the process. That's good, since different OS'es and even different OS versions might have more or fewer signals, or they may be renumbered, etc.

However, it means you can't set signal handling behaviors in your .lldbinit, and if you set them in one run they don't persist on re-run. That's been somewhat annoying over the years.

This adds a "DummySignals" map to the target that holds signals as std::strings, and the user intentions. I called them DummySignals because if you set them in an .lldbinit, they will go into the Dummy Target, and then will prime new targets. They work like all the other Dummy Target elements, if you add a handling with no Targets, they go to the dummy target then to your target. If you add them when there's a real target, it will only go into that target.

Just like before, if you run process handle when you have a process, the UnixSignals values will be changed, but the change will also be recorded in the DummySignals map so we can apply it on re-run.

I also added a -c option to "process handle" which allows you to clear the setting from your target (and with -d from the dummy target).
I also added a -t option to "process handle" to show you just the changes you made to the default setting values.

Since I wanted you to also be able to "reset to the default" I had to change the UnixSignals class so it recorded the default value as well as the current value.

There are a couple of pieces of this that aren't done yet, but the patch was getting pretty big.

  1. No SB API. I'll add an SBTarget::HandleSignal to set the handling in the dummy target.
  2. When you add a signal with no process, we can't spell-check the signal name for you. The current patch emits a warning on Launch or Attach if there was a signal whose handling is configured but the signal name wasn't recognized.

I don't plan to do #2 right now. You would have to have the AddSignal add the signal name to a global pool, and then construct all the Platforms and get them to make their UnixSignals so the registry would happen. Then you could compare the signal against this list.

I'm not even sure that being able to spell check the signals is worth having to construct all the platforms to build this registry.

Diff Detail

Event Timeline

jingham created this revision.May 23 2022, 5:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 5:21 PM
jingham requested review of this revision.May 23 2022, 5:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2022, 5:21 PM
clayborg added inline comments.May 23 2022, 6:04 PM
lldb/include/lldb/Target/Target.h
1421

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.

1429

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.

1533–1536

Switch to std::vector< UnixSignal::Signal> here and then we can prime the UnixSignals with the vector?

lldb/include/lldb/Target/UnixSignals.h
120

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.

126

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?

lldb/source/Commands/Options.td
758

Do we need the "Also" in the documentation here? Is this option only available when used with another option?

jingham marked 3 inline comments as done.EditedMay 24 2022, 10:00 AM

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. After all, 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). So pretending it knows a signal number would just be confusing. 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.

lldb/include/lldb/Target/Target.h
1421

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.

1429

See the comment above.

1533–1536

See the first comment.

lldb/include/lldb/Target/UnixSignals.h
120

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.

126

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.

lldb/source/Commands/Options.td
758

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.

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

lldb/include/lldb/Target/Target.h
1421

Makes sense.

jingham marked 3 inline comments as done.May 24 2022, 11:30 AM

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

Ah, I forgot about specifying the signal by number. Before you have a process I don't think we should allow signals by number. The mapping signal number -> "signal name for any platform" is not 1-1 so we couldn't guarantee we were doing the right thing here. I'll put in a check for "specified by number with no process" and error out.

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

Ah, I forgot about specifying the signal by number. Before you have a process I don't think we should allow signals by number. The mapping signal number -> "signal name for any platform" is not 1-1 so we couldn't guarantee we were doing the right thing here. I'll put in a check for "specified by number with no process" and error out.

I would add a test for this and make sure it fails gracefully in that case

jingham updated this revision to Diff 431745.May 24 2022, 12:07 PM

Don't allow setting signal actions by signal number before you have a process.

We don't know what signal 20 is going to end up being till we have a process, so allowing this by number doesn't make sense.

So the "process handle" command allows us to set signals by signal number as well. Does this patch support this? It seems like it wouldn't be too hard to do if we wanted to handle this. Lemme know what you think, other than that LGTM.

Ah, I forgot about specifying the signal by number. Before you have a process I don't think we should allow signals by number. The mapping signal number -> "signal name for any platform" is not 1-1 so we couldn't guarantee we were doing the right thing here. I'll put in a check for "specified by number with no process" and error out.

I would add a test for this and make sure it fails gracefully in that case

I added that. You get an error from the command saying you can't set signal actions by number w/o a process.

Don't allow setting signal actions by signal number before you have a process.

I understand

We don't know what signal 20 is going to end up being till we have a process, so allowing this by number doesn't make sense.

I am saying that it would be a good idea to make sure an error is returned when you do try and set a signal by number before a process exists and make sure there is a test that covers this if it isn't already in the current patch.

Don't allow setting signal actions by signal number before you have a process.

I understand

We don't know what signal 20 is going to end up being till we have a process, so allowing this by number doesn't make sense.

I am saying that it would be a good idea to make sure an error is returned when you do try and set a signal by number before a process exists and make sure there is a test that covers this if it isn't already in the current patch.

I think we crossed paths. I added that in the last update of the diff. The test is TestHandleProcess.py:20, and the code to enforce it starts around 1671 of CommandObjectProcess.cpp.

clayborg accepted this revision.May 24 2022, 4:21 PM

Don't allow setting signal actions by signal number before you have a process.

I understand

We don't know what signal 20 is going to end up being till we have a process, so allowing this by number doesn't make sense.

I am saying that it would be a good idea to make sure an error is returned when you do try and set a signal by number before a process exists and make sure there is a test that covers this if it isn't already in the current patch.

I think we crossed paths. I added that in the last update of the diff. The test is TestHandleProcess.py:20, and the code to enforce it starts around 1671 of CommandObjectProcess.cpp.

I see it, it is hard to tell what got updated. Thanks for the info.

This revision is now accepted and ready to land.May 24 2022, 4:21 PM
JDevlieghere added inline comments.May 24 2022, 7:23 PM
lldb/include/lldb/Target/Target.h
1451

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?

1533

Does it matter that these are ordered? Can this use a llvm::StringMap?

lldb/source/Commands/CommandObjectProcess.cpp
1477–1479

Let's initialize these to the same values as Clear.

1582

Not your change but why Target * and not Target &?

lldb/source/Target/Process.cpp
2940–2943

Clang format

lldb/source/Target/Target.cpp
293–294

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.

3365

It seems like this can take a std::stringand std::move it into the pair below. Or a StringRef if you turn this into a StringMap as per the other suggestion.

3374–3382

With a StringMap you can simplify all of this into:

auto& entry = m_dummy_signals[name];
entry.pass = pass;
entry.notify = notify;
...
3394–3397

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); });
3444–3446

Should this have been

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

Now signals_sp is never initialized.

3466–3472

This seems super useful. Maybe this function, together with the other utility I suggested, can go in a LazyBoolUtils or something in Utility.

JDevlieghere requested changes to this revision.May 24 2022, 7:24 PM
This revision now requires changes to proceed.May 24 2022, 7:24 PM
jingham updated this revision to Diff 432145.May 25 2022, 4:16 PM

Address Jonas' review comments

lldb/include/lldb/Target/Target.h
1451

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.

1533

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.

lldb/source/Commands/CommandObjectProcess.cpp
1477–1479

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.

1582

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

lldb/source/Target/Target.cpp
3365

I just changed the API to take a StringRef.

3374–3382

Nice.

3394–3397

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.

3444–3446

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.

3466–3472

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.

JDevlieghere accepted this revision.May 26 2022, 11:14 AM

LGTM

lldb/source/Commands/CommandObjectProcess.cpp
1477–1479

I vaguely remember at least one bug where we were using an uninitialized value, but maybe the problem there was that we didn't call OptionParsingStarting. If it's possible to forget to call that, then we should have an assert enforce that, but that's beyond the scope of this patch.

This revision is now accepted and ready to land.May 26 2022, 11:14 AM
jingham added inline comments.May 26 2022, 2:40 PM
lldb/source/Commands/CommandObjectProcess.cpp
1477–1479

OptionParsingStarting is called for the CommandObjects by the Command runner, so it shouldn't be possible to have that not happen. But this is indeed a requirement, so it would be worthwhile putting in some kind of assert for this if it's possible.

But the easier mistake to make (and one of the reasons why I'm still a little in favor of not initializing) is to initialize the variable when you add it to Options, but forget to add it to OptionsParsingStarting. Then you end up getting stale values, and since some of these can be object pointers, you can crash from that.

But I don't know how you would write an assert (short of source analysis) that "all ivars of this class must get reset in method X"...