Ignoring Signals via the API

Ok, I’ve attached a patch and inlined it below. Is git diff format ok?

I’ll probably need to do some more work on it as I need to be able to set the disposition before I even attach to a process. Any thoughts on how that can be accomplished?

0001-Add-the-ability-to-control-the-signal-disposition-vi.patch (13.3 KB)

That looks fine. A couple of comments.

It was cool that you added the Python enumerator! Those are very useful.

We generally use 'GetNumWhatever' and 'GetWhateverAtIndex" rather than "GetNext" till you get an invalid value for iteration, it would be good to keep with that convention.

It might be good to have the reverse of GetSignalNumberFromName, in case some systems have different names for the same "signal number". IIRC that did happen for some of the less common signals.

Thanks for working on this.

Jim

Ok, I've attached a patch and inlined it below. Is git diff format ok?

Yep.

I'll probably need to do some more work on it as I need to be able to set the disposition before I even attach to a process. Any thoughts on how that can be accomplished?

One way that would be handy would be to add new API to the SBLaunchInfo / SBAttachInfo.

class SBLaunchInfo {
....
  SBUnixSignals
  GetUnixSignals();

  void
  SetUnixSignals (SBUnixSignals unix_signals);
}

Repeat for SBAttachInfo. Then you could modify your signals to your liking and then launch or attach with the launch/attach info and it will do the right thing.

Greg

So Greg's suggestion is great if you are about to create a process, and just want to make and set the signals before you launch it. But that doesn't help command-line users who would like to do the equivalent of "process handle" without a process, and in any case is a by hand process, when you would like to make it automatic for all new processes.

The core of the problem is where you would store the changed disposition before you have a process that is going to use it. Do we make a phony "ur-target" or "ur-process" where we put information that will prime future targets/processes? In the case of breakpoint that should apply to all targets, I think we will have to do something like that. But for signals, I think the Platform is the entity that knows what the available signals for a process running on the platform are. So it should be the place to store the default and "user modified default" signal handling for processes run on that platform. Unfortunately, the Platform is in kind of a sketchy state at present, and doesn't have an SB presence at all. So there is some design work to be done before that is ready to be used.

Jim

Done. One comment though, I was thinking it’d be good to cache the list of signals in the SBUnixSignals object which both GetNumSignals and GetSignalAtIndex generates. Otherwise, the python enumerator is a O(n^2) operation. Probably not a real issue as there aren’t very many signals, but it’s just a nit.

0001-Add-the-ability-to-control-the-signal-disposition-vi.patch (14 KB)

That look great. Would I be too much of a pain if I were to ask for a test case?

Jim

I’m having some trouble running lldb’s test suite. I’m getting the error “AssertionError: False is not True : Current executable set successfully”.

I’ve attached a partially complete test, but since I can’t get the test suite working right now, it’s not been run.

0001-Add-a-unit-test-of-SBUnixSignals.patch (3.63 KB)

If you run dotest.py with the "-t" flag you'll see a bit more of what's going on during the test execution...

Off hand I can't see anything clearly wrong with the test..But I haven't run it.

That was super helpful, I didn’t know about -t. I’ve got one (hopefully last) issue with the test in that I believe I must be using the SBProcess.GetExitStatus() API incorrectly, as it always returns -1 for me. If that returned 0 when the debugee exited normally, the test would be finished. Thoughts?

0002-Add-a-unit-test-for-SBUnixSignals.patch (3.77 KB)

Hmm, so there's two possibilities:

  1. SIGINT is not being suppressed by LLDB and instead is causing an abrupt termination of the inferior, and the exit code is correctly reported.
  2. LLDB is incorrectly parsing the exit code of the subprocess.

Not sure how to narrow down what's happening here — if you can figure out what's going on, even if you don't have a fix, I recommend checking in the test. If it still isn't passing and is breaking some buildbots, maybe mark it with an @expectedFailureLinux and a corresponding comment pointing to an open llvm.org/bugs ticket.

Thanks for implementing tests for this feature Russel!

Cheers,
Dan

I poked around at this for a bit, and here’s what I found.

  1. The issue is probably a race condition. It only happens about 1/3 of the time on my machine.
  2. SIGINT is definitely being suppressed by lldb. I added a singal handler for SIGINT which prints a message to main.cpp, and no such message is printed when the test is run.

0001-Add-the-ability-to-control-the-signal-disposition-vi.patch (18.4 KB)

I'd still like to get this feature into the API if possible. An updated patch
set will be forthcoming momentarily. I'm no longer able to reproduce the race
that I had been awhile ago, so it'd be great if someone can take a look at it.

Thanks,
Russ Harmon

This commit allows you to control the signals that lldb will suppress, stop or
forward using the Python and C++ APIs.

Ping. Can someone take a look at this when they get a chance?

Hi Russell,

Can you put that on Phabricator (http://reviews.llvm.org/) and assign to me? Thanks!

-Todd

Looks good except for one thing: SBUnixSignals needs to keep a weak reference to the process:

class SBUnixSignals {
private:
    lldb::ProcessWP *m_opaque_wp;
};

Otherwise you can crash later if someone grabs a SBUnixSignals instance and uses it after the process gets destroyed.

These functions go away:

UnixSignals *
SBUnixSignals::GetPtr() const
{
    return m_opaque_ptr;
}

void
SBUnixSignals::SetPtr (UnixSignals *unix_signals_ptr)
{
    m_opaque_ptr = unix_signals_ptr;
}

And should probably be changed to:

lldb::ProcessSP
SBUnixSignals::GetSP() const
{
    return m_opaque_wp.lock();
}

void
SBUnixSignals::SetSP (const lldb::ProcessSP &process_sp)
{
    m_opaque_wp = process_sp;
}

Then all functions that want to do something, need to get the ProcessSP for the ProcessWP and then use it if the process shared pointer is valid:

bool
SBUnixSignals::IsValid() const
{
    return (bool)GetSP();
}

const char *
SBUnixSignals::GetSignalAsCString (int32_t signo) const
{
    ProcessSP process_sp(GetSP());
    if (process_sp)
        return process_sp.GetUnixSignals().GetSignalAsCString(signo);
    return NULL;
}

This will allow you to play with the process' unix signals object until the process doesn't exist anymore and then all functions become no-ops and everything stays safe. The only time it is ok to use pointers in the lldb::SB classes is for objects that are in global lists that never get destroyed, or for objects you create yourself and want to work on directly. Here we really want to be modifying the copy of the UnixSignals from a specific process, so we need to keep a smart reference to the process.

Greg

It shouldn't have a pointer to a weak pointer, it should have a instance of a Process weak_ptr...

Fixed class ivar definition:

class SBUnixSignals {
private:
   lldb::ProcessWP m_opaque_wp;
};