Re: [PATCH 1/4] 'catch syscall' feature -- Architecture-independent part

[Pedro Alves <pedro@codesourcery.com>]

On Tuesday 04 November 2008 04:31:19, Sérgio Durigan Júnior wrote:
> +static enum print_stop_action
> +print_it_catch_syscall (struct breakpoint *b)
> +{
> +  /* This is needed because we want to know in which state a
> +     syscall is.  It can be in the TARGET_WAITKIND_SYSCALL_ENTRY
> +     or TARGET_WAITKIND_SYSCALL_RETURN, and depending on it we
> +     must print "called syscall" or "returned from syscall".  */
> +  struct thread_info *th_info = find_thread_pid (inferior_ptid);
> +  const char *syscall_name =
> +    gdbarch_syscall_name_from_number (current_gdbarch, b->syscall_number);
> +
> +  annotate_catchpoint (b->number);
> +  printf_filtered (_("\nCatchpoint %d ("), b->number);
> +
> +  if (th_info->syscall_state == TARGET_WAITKIND_SYSCALL_ENTRY)
> +    printf_filtered (_("calling "));
> +  else
> +    printf_filtered (_("returned from "));

This bit left me wondering about the below.  Take these with a
grain of salt, please :-)

syscall_state has been placed in struct thread_info, and linux-nat.c
toggles it between entry/exit, but that's mainly because on linux, the
same trap is sent in both cases.  In the ttrace case (inf-ttrace.c), for
example, you have distinct TTEVT_SYSCALL_ENTRY and TTEVT_SYSCALL_RETURN
events at the target level.  Shouldn't we be doing the same on linux?  That
is, move 'syscall_state' to 'struct lwp_info', thus making it
private to the linux-nat.c implementation, and have the core side
always distinguish them from the TARGET_WAITKIND_SYSCALL_* type
returned from target_wait?  It looks weird for the target side to
be writing to a thread_info directly.  I always tend to ponder how I'd
do these things in gdbserver to validate target_ops designs --- I guess
I wouldn't be able to tweak gdb's common code from there.  :-)

Was it because you need to access it in print_it_catch_syscall?  You
could get it from the last target event like you do in
breakpoint_hit_catch_syscall, I guess.

(I guess the ideal solution would be for a bpstat to be able
to hold extra catchpoint hit state without re-querying the
last target event, but that would require an extra redesign of
the breakpoint hit -> bpstat building.)

Also, I'm not 100% sure, but I think you can crash in
linux-nat.c:linux_handle_extended_wait if an lwp happens to hit a syscall
you're catching before it's corresponding thread has been added to the
thread list in linux-thread-db.c.

Also, while we're on to speaking of these matters, would it make sense
to be able to catch only syscall entry or syscall return events
at the UI level?  That is, separate "catch syscall entry",
"catch syscall return" or some such?

-- 
Pedro Alves