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

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

Hi Daniel,

Thanks for your comments.

On Fri, 2009-02-27 at 17:11 -0500, Daniel Jacobowitz wrote:

> > > > +/* Implement the "print_one" breakpoint_ops method for syscall
> > > > +   catchpoints.  */
> > > > +
> > > > +static void
> > > > +print_one_catch_syscall (struct breakpoint *b, CORE_ADDR *last_addr)
> > > > +{
> > > 
> > > Have you tried hitting a syscall catchpoint in MI mode, and is the
> > > output anything useful?
> > 
> > No, unfortunately I haven't. Actually, I must first learn how to use the
> > MI interface, but that should not be hard :-).
> 
> I'd suggest doing that as part of this submission so that we know
> you're on the right track.  It isn't too hard; you can start by
> looking at the test logs from gdb.mi tests, if that helps.

All right, I'll do that today.

> > > > +# Fills the struct syscall (passed as argument) with the corresponding
> > > > +# system call represented by syscall_number.
> > > > +M:void:get_syscall_by_number:int syscall_number, struct syscall *s:syscall_number, s
> > > > +
> > > > +# Fills the struct syscall (passed as argument) with the corresponding
> > > > +# system call represented by syscall_name.
> > > > +M:void:get_syscall_by_name:const char *syscall_name, struct syscall *s:syscall_name, s
> > > > +
> > > > +# Returns the array containing the syscall names for the architecture.
> > > > +M:const char **:get_syscall_names:void:
> > > 
> > > If every target is going to use XML for this, these three do not need
> > > to be gdbarch methods and the support code can move from linux-tdep.c
> > > to xml-syscall.c.
> > 
> > As far as I understood (from our discussion a few months ago), not every
> > target is supposed to use the XML for syscalls. That's specially true
> > for embedded systems and/or architectures for which the XML file is
> > missing (for some obscure reason, don't know). That's why I thought it
> > would be better not to generalize.
> 
> I don't think this is a big deal.  If it is, we can handle it the same
> way as for target-descriptions: pre-compile them into GDB.

So I won't modify anything, ok?

> > > > +  if (target_passed_by_entrypoint () > 0
> > > > +      && catch_syscall_enabled () > 0)
> > > > +    request = PT_SYSCALL;
> > > > +  else
> > > > +    request = PT_CONTINUE;
> > > 
> > > Why is target_passed_by_entrypoint still necessary?  If we understand
> > > why, I think there'll be some other more appropriate flag.  Is it to
> > > avoid using PTRACE_SYSCALL when the shell is running, before the
> > > application starts?
> > 
> > It's been a long time since I added this check, but as far as I
> > remember, that's exactly the reason. I tried to remove this, and the
> > testcase simply freezes. Do you have another idea? :-)
> 
> Not sure that the flag exists any more, but you're trying to avoid it
> when called by startup_inferior.  I suppose you could use the
> inferior_created observer (not new_inferior!  The distinction is not
> too clear in the manual but that one is too early).  The problem is,
> again, that this flag needs to be per-inferior.
> 
> Pedro, any thoughts?

What do you mean by "Not sure that the flag exists any more"? Also, I'm
waiting for Pedro's reply.

> > > > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> > > > index 9a7e39c..1d0f66f 100644
> > > > --- a/gdb/linux-nat.c
> > > > +++ b/gdb/linux-nat.c
> > > > @@ -676,6 +676,7 @@ linux_child_post_attach (int pid)
> > > >  {
> > > >    linux_enable_event_reporting (pid_to_ptid (pid));
> > > >    check_for_thread_db ();
> > > > +  linux_enable_tracesysgood (pid_to_ptid (pid));
> > > >  }
> > > >  
> > > >  static void
> > > > @@ -683,6 +684,7 @@ linux_child_post_startup_inferior (ptid_t ptid)
> > > >  {
> > > >    linux_enable_event_reporting (ptid);
> > > >    check_for_thread_db ();
> > > > +  linux_enable_tracesysgood (ptid);
> > > >  }
> > > >  
> > > >  static int
> > > > @@ -4160,6 +4162,7 @@ linux_target_install_ops (struct target_ops *t)
> > > >    t->to_follow_fork = linux_child_follow_fork;
> > > >    t->to_find_memory_regions = linux_nat_find_memory_regions;
> > > >    t->to_make_corefile_notes = linux_nat_make_corefile_notes;
> > > > +  t->to_passed_by_entrypoint = linux_passed_by_entrypoint;
> > > >  
> > > >    super_xfer_partial = t->to_xfer_partial;
> > > >    t->to_xfer_partial = linux_xfer_partial;
> > > 
> > > These bits must be for another patch in the series :-)
> > 
> > I'm sorry, I didn't understand what you meant by that :-(. These
> > modifications are all architecture-independent, so this is the right
> > place for them right?
> 
> No - since they're specific to Linux.  Also, I don't think they'll
> compile at this point, you haven't added the function yet.

Well, I know it's a shame, but the patches for catch syscall don't
compile alone at all. You must have all four patches in order to get
things working, and I haven't even tested if the patches can be compiled
independently. However, you're right regarding this piece of code: it
should be placed in other patch.

Regards,

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