Re: [PATCH v4] Implement 'catch syscall' for gdbserver

[Josh Stone <jistone@redhat.com>]

On 01/12/2016 04:05 AM, Pedro Alves wrote:
> On 01/09/2016 03:09 AM, Josh Stone wrote:
> 
>>
>> 2016-01-08  Josh Stone  <jistone@redhat.com>
>> 	    Philippe Waroquiers  <philippe.waroquiers@skynet.be>
>>
>> 	* gdb.texinfo (Remote Configuration): List the QCatchSyscalls packet.
>> 	(Stop Reply Packets): List the syscall entry and return stop reasons.
>> 	(General Query Packets): Describe QCatchSyscalls, and add it to the
>> 	table and detailed list of stub features.
>>
> 
> "table of detailed", I think.

I'm referring to two hunks:
- the table: Feature Name / Value Required / Default / Probe Allowed
- the list below it, "currently defined stub features, in more detail"

Maybe I just need another article, "to the table and the detailed list"

>> @@ -648,6 +658,12 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
>>        event_thr->last_resume_kind = resume_continue;
>>        event_thr->last_status.kind = TARGET_WAITKIND_IGNORE;
>>  
>> +      /* Update syscall state in the new lwp, effectively mid-syscall too.
>> +	 The client really should send a new list to catch, in case the
>> +	 architecture changed, but for ANY_SYSCALL it doesn't matter.  */
>> +      event_lwp->syscall_state = TARGET_WAITKIND_SYSCALL_ENTRY;
>> +      proc->syscalls_to_catch = syscalls_to_catch;
> 
> The tone of this comment sounds to me as if the client should always
> send a new list, just in case, but for some odd reason it sometimes doesn't.
> 
> I think we want to convey the opposite, like:
> 
>          /* Update syscall state in the new lwp, effectively mid-syscall too.  */
>          event_lwp->syscall_state = TARGET_WAITKIND_SYSCALL_ENTRY;
> 
>          /* Restore the list to catch.  Don't rely on the client, which is free
>             to avoid sending a new list when the architecture doesn't change.
>             Also, for ANY_SYSCALL, the architecture doesn't really matter.  */
>          proc->syscalls_to_catch = syscalls_to_catch;

Sure, I'll take your rewrite verbatim, if you don't mind.

>>  static int
>> +linux_supports_catch_syscall (void)
>> +{
>> +  return (the_low_target.get_syscall_trapinfo != NULL
>> +	  && linux_supports_tracesysgood());
> 
> Space: "linux_supports_tracesysgood ()"

OK

>> +proc test_catch_syscall_execve {} {
>> +    global gdb_prompt decimal
>> +
>> +    with_test_prefix "execve" {
>> +
>> +	# Tell the test program we want an execve.
>> +	gdb_test_no_output "set do_execve = 1"
>> +
>> +	# Check for entry/return across the execve, making sure that the
>> +	# syscall_state isn't lost when turning into a new process.
>> +	insert_catch_syscall_with_arg "execve"
>> +	check_continue "execve"
>> +
>> +	# Remotes that don't track exec may report the raw SIGTRAP for it.
>> +	# If we use stepi now, we'll get a consistent trap for all targets.
>> +	gdb_test "stepi" ".*" "step after execve"
> 
> Why is it important to do this raw SIGTRAP handling?  What happens if you don't
> do this?  Won't those targets already FAIL the check_continue tests?

Just in case, the context from Linux man ptrace:

  If the PTRACE_O_TRACEEXEC option is not in effect for the execing
  tracee, and if the tracee was PTRACE_ATTACHed rather that
  PTRACE_SEIZEd, the kernel delivers an extra SIGTRAP to the tracee
  after execve(2) returns.  This is an ordinary signal (similar to
  one which can be generated by kill -TRAP), not a special kind of
  ptrace-stop.

Since that's a signal-stop *after* execve returns, the check_continue
will have succeeded already.

The check_continue is really the only bit I care about for this test
anyway.  The rest is just trying to finish the target process cleanly.
I was having trouble matching consistent output since plain remote was
getting that SIGTRAP, but extended-remote would use exec events and not
report anything extra.  Adding the stepi made both stop the same way.

This is moot now, since plain remotes are now tracking exec events too.
 I developed this test just before that went in last month. :)
I just tried with that stepi commented out, and the test still passes on
local, remote, and extended-remote, so I'll remove it.