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

[Pedro Alves <palves@redhat.com>]

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.


> @@ -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;

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

Space: "linux_supports_tracesysgood ()"



>  
> +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?

Thanks,
Pedro Alves