[RFA/Linux] Ask kernel to kill inferior when GDB terminates

  Hello,

This patch enhances GDB on GNU/Linux systems to ask the kernel
to kill the inferior if GDB terminates without doing it itself.
This would typically happen when GDB encounters a problem and
crashes, or when it gets killed by an external process. This can
be observed by starting a program under GDB, and then killing
GDB with signal 9. After GDB is killed, the inferior still remains
in "interruptible sleep (waiting for an event to complete)" state.

gdb/ChangeLog:

        * nat/linux-ptrace.h (PTRACE_O_EXITKILL): Define if not
        already defined.
        * nat/linux-ptrace.c (linux_test_for_exitkill): New advance
        declaration.  New function.
        (linux_check_ptrace_features): Call linux_test_for_exitkill.

Tested on various x86_64-linux and x86-linux distributions
(RHES5, RHES6, RHES7) and my x86_64-linux machine, which runs
Ubuntu 14.04. On older versions of the kernel, the problem
persists, since the feature is not available, while the inferior
now automatically gets killed on newer versions of the kernel.

OK to commit?

PS: On a side note, the use of advance declarations for those
"linux_test_for_[...]" functions isn't really necessary, if we
were to move the body of those functions ahaad of
linux_check_ptrace_features. For now, I went with consistency,
but I am more than happy providing a followup patch that does
the move.

Thanks,
  

On 11/14/2014 04:53 PM, Joel Brobecker wrote:
> Hello,
> 
> This patch enhances GDB on GNU/Linux systems to ask the kernel
> to kill the inferior if GDB terminates without doing it itself.
> This would typically happen when GDB encounters a problem and
> crashes, or when it gets killed by an external process. This can
> be observed by starting a program under GDB, and then killing
> GDB with signal 9. After GDB is killed, the inferior still remains
> in "interruptible sleep (waiting for an event to complete)" state.

I could see this making some sense when GDB has spawned the process,
but it seems harsh when GDB has attached to the process instead
of spawning it?

Note that Windows has had a similar feature for
ages (DebugSetProcessKillOnExit), and how windows-nat.c calls
DebugSetProcessKillOnExit(false) when GDB attaches to a process.

Thanks,
Pedro Alves
  

> I could see this making some sense when GDB has spawned the process,
> but it seems harsh when GDB has attached to the process instead
> of spawning it?

Hmmm, good point. I'd like to verify what happens in that case, and
whether the process remains stopped or if execution resumes in that
case.

> Note that Windows has had a similar feature for
> ages (DebugSetProcessKillOnExit), and how windows-nat.c calls
> DebugSetProcessKillOnExit(false) when GDB attaches to a process.

Thanks, Pedro.
  

> > I could see this making some sense when GDB has spawned the process,
> > but it seems harsh when GDB has attached to the process instead
> > of spawning it?
> 
> Hmmm, good point. I'd like to verify what happens in that case, and
> whether the process remains stopped or if execution resumes in that
> case.

I took a little bit of time to experiment today. The experimentation
was conducted with Linux 3.13.0-39-generic from x86_64 Ubuntu.
In both cases, when GDB runs the program or attaches to a program,
killing GDB while the inferior is still running detaches GDB from
the inferior, which means it keeps running.

I will adapt my patch when I have a moment.
  

On 11/19/2014 09:25 AM, Joel Brobecker wrote:
>>> I could see this making some sense when GDB has spawned the process,
>>> but it seems harsh when GDB has attached to the process instead
>>> of spawning it?
>>
>> Hmmm, good point. I'd like to verify what happens in that case, and
>> whether the process remains stopped or if execution resumes in that
>> case.
> 
> I took a little bit of time to experiment today. The experimentation
> was conducted with Linux 3.13.0-39-generic from x86_64 Ubuntu.
> In both cases, when GDB runs the program or attaches to a program,
> killing GDB while the inferior is still running detaches GDB from
> the inferior, which means it keeps running.

Sorry, I didn't realize you meant to verify what happens without
the patch.  Yeah, it's because the ptracer always detaches that
the option was introduced, not that long ago.

Note that with PTRACE_TRACEME there's still a time window between
starting to trace a task and setting its ptrace options, where
GDB could crash, and thus we can still miss killing the tracee.  To
completely close that, we'd have to switch to use the newer PTRACE_SEIZE
instead, which takes the ptrace options as argument, but that'd be
a story of its own.

> I will adapt my patch when I have a moment.

Thank you.

Pedro Alves
  

> >>> I could see this making some sense when GDB has spawned the process,
> >>> but it seems harsh when GDB has attached to the process instead
> >>> of spawning it?
[...]
> > I will adapt my patch when I have a moment.

Just took a look, and it's not clear to me what the best way to do that
might me. For native GDB, it'd be fairly easy, if I could just add
an "attaching_p" parameter to "linux_enable_event_reporting", then
I could just mask PTRACE_O_EXITKILL out if nonzero. In linux-nat.c,
it's fairly easy to provide that info. However, things are not so
clear in the gdbserver case, because linux_enable_event_reporting
is performed during the event handling conditional on
child->must_set_ptrace_flags:

  if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
    {
      linux_enable_event_reporting (lwpid);
      child->must_set_ptrace_flags = 0;
    }

I think the only clean way to do this would be to add another
field in struct lwp_info such as "attaching_p" which we would
set to 1 in the attaching case only.

Does this seem reasonable?

Thanks,
  

On 12/13/2014 04:27 PM, Joel Brobecker wrote:
>>>>> I could see this making some sense when GDB has spawned the process,
>>>>> but it seems harsh when GDB has attached to the process instead
>>>>> of spawning it?
> [...]
>>> I will adapt my patch when I have a moment.
> 
> Just took a look, and it's not clear to me what the best way to do that
> might me. For native GDB, it'd be fairly easy, if I could just add
> an "attaching_p" parameter to "linux_enable_event_reporting", then
> I could just mask PTRACE_O_EXITKILL out if nonzero. In linux-nat.c,
> it's fairly easy to provide that info. However, things are not so
> clear in the gdbserver case, because linux_enable_event_reporting
> is performed during the event handling conditional on
> child->must_set_ptrace_flags:
> 
>   if (WIFSTOPPED (wstat) && child->must_set_ptrace_flags)
>     {
>       linux_enable_event_reporting (lwpid);
>       child->must_set_ptrace_flags = 0;
>     }
> 
> I think the only clean way to do this would be to add another
> field in struct lwp_info such as "attaching_p" which we would
> set to 1 in the attaching case only.

I think we can get by without adding a new field.  The process_info
structure has the "process->attached" field already.  We could
check that here.  Alternatively, must_set_ptrace_flags could be a
tri-state instead of a boolean, which would be cheaper.

> 
> Does this seem reasonable?
> 
> Thanks,
> 


Thanks,
Pedro Alves