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

["Sérgio Durigan Júnior" <sergiodj@linux.vnet.ibm.com>]

Hi Pedro,

On Tue, 2008-11-04 at 16:17 +0000, Pedro Alves wrote:
> 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 :-)

Of course :-). And I'm sorry for taking so long to respond.

> 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.

Ok, I'll try to put the syscall_state in 'struct lwp_info'. Honestly, I
don't remember now why I've chosen to put this variable inside
thread_info, but of course you're way more capable of telling me how to
make my design be more clever (and look more like GDB) :-).

> 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.

I'm not sure I'm able to answer this, but that's a problem that I wasn't
considering. Thanks for the hint.

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

IMHO, we could postpone this for now. Honestly I'm pretty satisfied with
the feature right now, but of course my opinion shouldn't count that
much ;-).

Thanks for your comments, I'll fix my code as soon as I can.

Regards, 

-- 
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil