Re: [PATCH v3 2/2] Implement 'catch syscall' for gdbserver

[Pedro Alves <palves@redhat.com>]

Hi Josh.

On 12/11/2015 09:19 PM, Josh Stone wrote:
> On 12/04/2015 06:16 PM, Josh Stone wrote:
>> On 12/04/2015 05:18 AM, Pedro Alves wrote:
>>> Quick question: What is supposed to happen to the QCatchSyscalls
>>> when the process execs?  I'm thinking of 64-bit inferior execing
>>> 32-bit inferior, etc.  The syscall numbers aren't necessarily shared
>>> between the different architectures.  This implementation deletes discards
>>> the previous QCatchSyscalls on exec, and I think that's what makes sense,
>>> but, I think that this should be explicit in the packet's description.
>>
>> Yes, I think it should be cleared to avoid any assumption about the
>> architecture.  I'll add a note in the description codifying this.
> 
> After exploration, I'm having second thoughts about this point.  Yes,
> the current implementation clears it, but only when PTRACE_O_TRACEEXEC
> is enabled to actually get that event.  That's only if the client sent
> "qSupports:exec-events+".  Otherwise, the server doesn't even know an
> exec happened, so it can't really promise to reset the syscall table.

Hmm.  It sounds reasonable.

However, I think that gdb will always want to listen to exec events.
It has to, in order to install breakpoints in the new image, and make sure
that it doesn't incorrectly remove old breakpoints in the new image either
the next time the program stops.  That older gdb's weren't aware of execs
is plain and simply a bug.

I understand you may be thinking of strace instead of gdb though,
which mainly only cares about syscalls.

I think even strace always need to handle exec events too, to handle
the case that the exec event is reported to the thread group
leader, even if it was some other thread that execed.

But I see the point that this may avoid extra stops and traffic
in some scenarios, so I'm fine with specifying it that way.

> 
> Since the server doesn't promise to always catch execs, I think we
> should actually go the other way to stay consistent.  

If the server doesn't catch execs, then gdb won't know about them either,
and so from gdb's perpective, the syscalls-to-catch-list doesn't change
either.  It's as-if the exec didn't happen, so the server would naturally
catch syscalls across the exec.  Doesn't seem to me that there'd be an
issue here.

> Let the syscall
> list be carried over, and document that clients should probably send a
> new list after execs in case the architecture changed.  Some clients may
> just choose to live with the assumption that the arch is consistent in
> their environment.

We really should say "probably" in the docs.  Stub authors won't know
what to do then.  We should be definitive.

On the gdb side, I think that determining whether the arch changed would
be somewhat a complication -- the place we discover the new arch isn't
the same place we install new breakpoints.  We could perhaps have the core
always install the catchpoints and then we could handle that
by making remote.c remember the last arch the syscalls list was set
to for the given inferior, and compare that arch with the inferior's
current arch, somehow generically.

BTW, am I right that with your current patch, if the user sets
10 syscall catchpoints, remote_set_syscall_catchpoint will
be called 10 times, and thus gdbserver gets 10 QCatchSyscalls
packets?

> 
>>> I'm not sure gdb clears the inferior's "syscalls to the caught" VEC
>>> on exec events, but it probably does (if it doesn't, I think it should.)
>>
>> I'll see if I can find out.
> 
> AFAICT the only time anything is removed from syscall_catchpoint's VEC
> syscalls_to_be_caught is in breakpoint_ops->remove_location and ->dtor,
> respectively remove_catch_syscall and dtor_catch_syscall.  And since
> this list isn't stored in the lwp structure itself, the exec doesn't
> really affect anything.  Right?
> 

Hmm.  Then it sounds like we still mishandle e.g., "catch syscall open"
across execs that change archs, as described at:

 https://sourceware.org/bugzilla/show_bug.cgi?id=10737#c5

Thanks,
Pedro Alves