[v2,1/2] gdbserver: Set Linux ptrace options ASAP

  The ptrace options should be set as soon as we know a thread is stopped,
so no events can be missed.  There's an arch-setup early return that was
effectively delaying this update before, and I found for instance that
the first syscall event wouldn't be properly reported with TRACESYSGOOD.
It's now more similar to the way that gdb/linux-nat.c handles it.

gdb/gdbserver/ChangeLog:

2015-11-25  Josh Stone  <jistone@redhat.com>

	* linux-low.c (linux_low_filter_event): Set ptrace options as soon as
	each thread is stopped, even before arch-specific setup.
---
 gdb/gdbserver/linux-low.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
  

On 11/26/2015 02:53 AM, Josh Stone wrote:
> The ptrace options should be set as soon as we know a thread is stopped,
> so no events can be missed.  There's an arch-setup early return that was
> effectively delaying this update before, and I found for instance that
> the first syscall event wouldn't be properly reported with TRACESYSGOOD.
> It's now more similar to the way that gdb/linux-nat.c handles it.

Hmm, I'm confused on how this resulted in the first syscall being misssed.
That early return happens when we're not executing the real inferior
yet -- the process is still running the "gdbserver --wrapper WRAPPER"
binary.

It's pedantically good, though not crucial, to set PTRACE_O_TRACEEXEC early for
that scenario, to get a real PTRACE_EVENT_EXEC event instead of a bare SIGTRAP
when the exec wrapper (or in the future, the shell, when we start inferiors
with the shell, like gdb does, for arg expansion and globbing) actually execs.

If the shell/wrapper forks, enabling fork events while still executing the
wrapper/shell breaks startup -- server.c:start_inferior.  The gdb
version (fork-child.c:startup_inferior) does handle TARGET_WAITKIND_FORKED,
but AFAICS forgets detaching/resuming the child...

We _must_ not catch syscall events while running the exec wrapper (or
the shell), otherwise server.c:start_inferior would get confused for seeing
unexpected syscall stops.  If the backend treats syscall catchpoints, it's OK,
since gdb won't insert catchpoints in the process until after vRun returns,
indicating the process is stopped at the entry point.  IIRC, gdb actually
does NOT handle catchpoint locations per-inferior today, but as long as
the backend side thinks of catchpoints per-inferior, we can fix the GDB side.

So all in all, I'm not sure this actually buys us anything other than need
to fix the wrapper/shell-forks case.

> 
> gdb/gdbserver/ChangeLog:
> 
> 2015-11-25  Josh Stone  <jistone@redhat.com>
> 
> 	* linux-low.c (linux_low_filter_event): Set ptrace options as soon as
> 	each thread is stopped, even before arch-specific setup.

Thanks,
Pedro Alves
  

On 11/26/2015 02:34 AM, Pedro Alves wrote:
> On 11/26/2015 02:53 AM, Josh Stone wrote:
>> The ptrace options should be set as soon as we know a thread is stopped,
>> so no events can be missed.  There's an arch-setup early return that was
>> effectively delaying this update before, and I found for instance that
>> the first syscall event wouldn't be properly reported with TRACESYSGOOD.
>> It's now more similar to the way that gdb/linux-nat.c handles it.
> 
> Hmm, I'm confused on how this resulted in the first syscall being misssed.
> That early return happens when we're not executing the real inferior
> yet -- the process is still running the "gdbserver --wrapper WRAPPER"
> binary.

My memory of this is admittedly hazy by now.  IIRC the first syscall
wasn't *completely* missed, just reported without TRACESYSGOOD in
effect, so it looked like a plain SIGTRAP.

I will try to dig in and characterize the problem I had better,
especially with your explanation of exec startup at hand.  Thanks!

> It's pedantically good, though not crucial, to set PTRACE_O_TRACEEXEC early for
> that scenario, to get a real PTRACE_EVENT_EXEC event instead of a bare SIGTRAP
> when the exec wrapper (or in the future, the shell, when we start inferiors
> with the shell, like gdb does, for arg expansion and globbing) actually execs.
> 
> If the shell/wrapper forks, enabling fork events while still executing the
> wrapper/shell breaks startup -- server.c:start_inferior.  The gdb
> version (fork-child.c:startup_inferior) does handle TARGET_WAITKIND_FORKED,
> but AFAICS forgets detaching/resuming the child...
> 
> We _must_ not catch syscall events while running the exec wrapper (or
> the shell), otherwise server.c:start_inferior would get confused for seeing
> unexpected syscall stops.  If the backend treats syscall catchpoints, it's OK,
> since gdb won't insert catchpoints in the process until after vRun returns,
> indicating the process is stopped at the entry point.  IIRC, gdb actually
> does NOT handle catchpoint locations per-inferior today, but as long as
> the backend side thinks of catchpoints per-inferior, we can fix the GDB side.
> 
> So all in all, I'm not sure this actually buys us anything other than need
> to fix the wrapper/shell-forks case.
> 
>>
>> gdb/gdbserver/ChangeLog:
>>
>> 2015-11-25  Josh Stone  <jistone@redhat.com>
>>
>> 	* linux-low.c (linux_low_filter_event): Set ptrace options as soon as
>> 	each thread is stopped, even before arch-specific setup.
> 
> Thanks,
> Pedro Alves
>
  

On 11/30/2015 10:50 AM, Josh Stone wrote:
> On 11/26/2015 02:34 AM, Pedro Alves wrote:
>> On 11/26/2015 02:53 AM, Josh Stone wrote:
>>> The ptrace options should be set as soon as we know a thread is stopped,
>>> so no events can be missed.  There's an arch-setup early return that was
>>> effectively delaying this update before, and I found for instance that
>>> the first syscall event wouldn't be properly reported with TRACESYSGOOD.
>>> It's now more similar to the way that gdb/linux-nat.c handles it.
>>
>> Hmm, I'm confused on how this resulted in the first syscall being misssed.
>> That early return happens when we're not executing the real inferior
>> yet -- the process is still running the "gdbserver --wrapper WRAPPER"
>> binary.
> 
> My memory of this is admittedly hazy by now.  IIRC the first syscall
> wasn't *completely* missed, just reported without TRACESYSGOOD in
> effect, so it looked like a plain SIGTRAP.
> 
> I will try to dig in and characterize the problem I had better,
> especially with your explanation of exec startup at hand.  Thanks!

OK, I think I've got it, and it's a real regression from 7.10 even for
other events like fork.  I'm not using --wrapper, so I'm not sure of the
interaction there, but even gdbserver's simple fork+exec can show the
problem.  Basically, on the very first stop we don't set flags yet, so
the first resume from there continues without the right flags.

The sequence I was running into with syscalls goes like this:

- start_inferior calls create_inferior to fork, then calls mywait
  - the forked process calls ptrace(PTRACE_TRACEME), then execs
- linux_low_filter_event sees a raw SIGTRAP for the child after exec
  - (we haven't set PTRACE_O_TRACEEXEC yet, so SIGTRAP is expected)
  - arch setup is needed, so it hits the early return (new since 7.10)
    ... thus child->must_set_ptrace_flags is not dealt with
- start_inferior calls target_arch_setup
- GDB sends QCatchSyscalls:1
- linux_resume_one_lwp_throw calls ptrace(PTRACE_SYSCALL)
  - but we still haven't set any flags, especially PTRACE_O_TRACESYSGOOD
- linux_low_filter_event sees a raw SIGTRAP for the first syscall entry
  - now we finally deal with child->must_set_ptrace_flags
- linux_resume_one_lwp_throw calls ptrace(PTRACE_SYSCALL)
- linux_low_filter_event sees SYSCALL_SIGTRAP for the return
  - entry/return logic is confused now, thinks this is an entry
  - (but if there's any other event, entry/return will get back in sync)


But this problem isn't particular to my syscall patches.  Consider this
simple forking program and use 'catch fork':

  #include <unistd.h>
  int main() { fork(); return 0; }

Compiled normally, with dynamically-linked libc et al, you get:
- SIGTRAP after exec, ignores child->must_set_ptrace_flags.
- SIGTRAP for a swbreak, I guess some gdb setup, then it sets the
necessary flags, especially PTRACE_O_TRACEFORK.
- SIGTRAP for PTRACE_EVENT_FORK, hooray!

But compiled statically:
- SIGTRAP after exec, ignores child->must_set_ptrace_flags.
- CLD_EXITED, flags were never set!
- if I add a breakpoint on main, flags will be set when that's reached,
and then we do get the PTRACE_EVENT_FORK after all.


So, we need some point to get the right flags set before the program
starts running for real.  If you don't like the way I moved the flags
before that arch-setup early return, then when should we do it?

- Perhaps before the ptrace call in linux_resume_one_lwp_throw?  Then if
any state changes while the thread is still stopped, triggering new
must_set_ptrace_flags, we'll deal with it before resuming.  But I don't
know if this would interact well with your wrapper concerns.

- Perhaps at the end of linux_arch_setup?  AIUI this will be after
everything you're worried about wrappers.


>> It's pedantically good, though not crucial, to set PTRACE_O_TRACEEXEC early for
>> that scenario, to get a real PTRACE_EVENT_EXEC event instead of a bare SIGTRAP
>> when the exec wrapper (or in the future, the shell, when we start inferiors
>> with the shell, like gdb does, for arg expansion and globbing) actually execs.
>>
>> If the shell/wrapper forks, enabling fork events while still executing the
>> wrapper/shell breaks startup -- server.c:start_inferior.  The gdb
>> version (fork-child.c:startup_inferior) does handle TARGET_WAITKIND_FORKED,
>> but AFAICS forgets detaching/resuming the child...
>>
>> We _must_ not catch syscall events while running the exec wrapper (or
>> the shell), otherwise server.c:start_inferior would get confused for seeing
>> unexpected syscall stops.  If the backend treats syscall catchpoints, it's OK,
>> since gdb won't insert catchpoints in the process until after vRun returns,
>> indicating the process is stopped at the entry point.  IIRC, gdb actually
>> does NOT handle catchpoint locations per-inferior today, but as long as
>> the backend side thinks of catchpoints per-inferior, we can fix the GDB side.
>>
>> So all in all, I'm not sure this actually buys us anything other than need
>> to fix the wrapper/shell-forks case.
>>
>>>
>>> gdb/gdbserver/ChangeLog:
>>>
>>> 2015-11-25  Josh Stone  <jistone@redhat.com>
>>>
>>> 	* linux-low.c (linux_low_filter_event): Set ptrace options as soon as
>>> 	each thread is stopped, even before arch-specific setup.
>>
>> Thanks,
>> Pedro Alves
>>
>
  

On 12/01/2015 08:17 PM, Josh Stone wrote:

> OK, I think I've got it, and it's a real regression from 7.10 even for
> other events like fork.  I'm not using --wrapper, so I'm not sure of the
> interaction there, but even gdbserver's simple fork+exec can show the
> problem.  Basically, on the very first stop we don't set flags yet, so
> the first resume from there continues without the right flags.
> 
> The sequence I was running into with syscalls goes like this:
> 
> - start_inferior calls create_inferior to fork, then calls mywait
>   - the forked process calls ptrace(PTRACE_TRACEME), then execs
> - linux_low_filter_event sees a raw SIGTRAP for the child after exec
>   - (we haven't set PTRACE_O_TRACEEXEC yet, so SIGTRAP is expected)
>   - arch setup is needed, so it hits the early return (new since 7.10)
>     ... thus child->must_set_ptrace_flags is not dealt with
> - start_inferior calls target_arch_setup
> - GDB sends QCatchSyscalls:1
> - linux_resume_one_lwp_throw calls ptrace(PTRACE_SYSCALL)
>   - but we still haven't set any flags, especially PTRACE_O_TRACESYSGOOD
> - linux_low_filter_event sees a raw SIGTRAP for the first syscall entry
>   - now we finally deal with child->must_set_ptrace_flags
> - linux_resume_one_lwp_throw calls ptrace(PTRACE_SYSCALL)
> - linux_low_filter_event sees SYSCALL_SIGTRAP for the return
>   - entry/return logic is confused now, thinks this is an entry
>   - (but if there's any other event, entry/return will get back in sync)
> 
> 
> But this problem isn't particular to my syscall patches.  Consider this
> simple forking program and use 'catch fork':
> 
>   #include <unistd.h>
>   int main() { fork(); return 0; }
> 
> Compiled normally, with dynamically-linked libc et al, you get:
> - SIGTRAP after exec, ignores child->must_set_ptrace_flags.
> - SIGTRAP for a swbreak, I guess some gdb setup, then it sets the
> necessary flags, especially PTRACE_O_TRACEFORK.
> - SIGTRAP for PTRACE_EVENT_FORK, hooray!
> 
> But compiled statically:
> - SIGTRAP after exec, ignores child->must_set_ptrace_flags.
> - CLD_EXITED, flags were never set!
> - if I add a breakpoint on main, flags will be set when that's reached,
> and then we do get the PTRACE_EVENT_FORK after all.

Ouch.  Thanks, I'm clear now.   It'd be super if one of these examples
got converted to a test case.

> 
> So, we need some point to get the right flags set before the program
> starts running for real.  If you don't like the way I moved the flags
> before that arch-setup early return, then when should we do it?
> 
> - Perhaps before the ptrace call in linux_resume_one_lwp_throw?  Then if
> any state changes while the thread is still stopped, triggering new
> must_set_ptrace_flags, we'll deal with it before resuming.  But I don't
> know if this would interact well with your wrapper concerns.
> 

Yeah, badly.

> - Perhaps at the end of linux_arch_setup?  AIUI this will be after
> everything you're worried about wrappers.

Something like that, yes.  gdb/linux-nat.c also does something like
that:

static void
linux_child_post_startup_inferior (struct target_ops *self, ptid_t ptid)
{
  linux_init_ptrace (ptid_get_pid (ptid), 0);
}

I think we should rename gdbserver's target_ops:target_arch_setup method/hook
to target_post_create_inferior along the way, and then linux-low.c's
implementation can both call linux_arch_setup and set the ptrace options.
Only Linux implements that target_ops method currently, so it should
be trivial.

Thanks,
Pedro Alves