This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/4] catch syscall -- try 4 -- Architecture-independent part
- From: Sérgio Durigan Júnior <sergiodj at linux dot vnet dot ibm dot com>
- To: Daniel Jacobowitz <drow at false dot org>
- Cc: Pedro Alves <pedro at codesourcery dot com>, gdb-patches at sourceware dot org, teawater <teawater at gmail dot com>
- Date: Wed, 25 Feb 2009 08:26:29 -0300
- Subject: Re: [PATCH 1/4] catch syscall -- try 4 -- Architecture-independent part
- References: <1232929831.26873.22.camel@miki> <200901260053.06295.pedro@codesourcery.com> <1232945747.26873.27.camel@miki> <1232989355.26873.39.camel@miki> <20090201193306.GJ4597@caradoc.them.org>
Hi Daniel,
A few questions before I'm able to resubmit the patch. Thanks for your
review, by the way.
On Sun, 2009-02-01 at 14:33 -0500, Daniel Jacobowitz wrote:
> Hi Sergio,
>
> On Mon, Jan 26, 2009 at 03:02:34PM -0200, Sérgio Durigan Júnior wrote:
> > +/* Used in run_command to reset syscall catchpoints fields. */
> > +
> > +void
> > +clear_syscall_catchpoints_info (void)
> > +{
> > + struct breakpoint *b;
> > +
> > + ALL_BREAKPOINTS (b)
> > + if (syscall_catchpoint_p (b))
> > + b->syscall_number = UNKNOWN_SYSCALL;
> > +}
> > +
> > /* Used in run_command to zero the hit count when a new run starts. */
> >
> > void
>
> I see that the other catchpoints put mutable data in struct breakpoint
> too. But it's really not a good idea :-(
>
> In non-stop GDB, suppose we have two threads running. They make
> different system calls and stop at the catchpoint. There's only one
> 'struct breakpoint', though, so do we report that they've both made
> the same system call even though they might not have?
>
> Can this go somewhere else? There's already a copy in the waitstatus,
> maybe it can stay there if we copy the whole waitstatus into 'struct
> thread_info', instead of copying just stop_signal.
I'm not sure I understood what you said, so please correct me if I'm
wrong. IIUC, you want me to create another field in 'struct
thread_info', and this field would be a 'struct target_waitstatus',
right? This is because you think I could copy the whole 'struct
target_waitstatus' in 'deal_with_syscall_event', and use it someplace
else. Is it right? If it is, I have a question: would I have access to
the current 'struct thread_info' inside breakpoint.c?
> > + get_last_target_status (&ptid, &last);
> > +
> > + annotate_catchpoint (b->number);
> > +
> > + if (s.name == NULL)
> > + syscall_id = xstrprintf ("%d", b->syscall_number);
> > + else
> > + syscall_id = xstrprintf ("'%s'", s.name);
>
> For instance, here we've got the waitstatus in addition to the breakpoint.
That's what confused me. I didn't get if you want me to use 'struct
thread_info' or 'struct target_waitstatus' to hold the syscall_number
info :-).
> > +/* 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 :-).
> > + if (s.name == NULL)
> > + /* We can issue just a warning, but still create the catchpoint.
> > + This is because, even not knowing the syscall name that
> > + this number represents, we can still try to catch the syscall
> > + number. */
> > + warning (_("The number '%d' does not represent a known syscall."),
> > + syscall_number);
>
> I don't think this warning is useful; if they're entering numbers,
> hopefully they know what they're doing.
Yeah, it's because of the "hopefully" part that I decided to issue this
warning. But that's not a big deal; I'll remove.
> > +# 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.
> > + 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? :-)
> > 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?
> > diff --git a/gdb/main.c b/gdb/main.c
> > index 0eb9596..a4405e7 100644
> > --- a/gdb/main.c
> > +++ b/gdb/main.c
> > @@ -62,6 +62,9 @@ int dbx_commands = 0;
> > /* System root path, used to find libraries etc. */
> > char *gdb_sysroot = 0;
> >
> > +/* GDB datadir, used to store data files. */
> > +char *gdb_datadir = 0;
> > +
>
> This is an example of a piece of the patch which could be separated
> out easily. Locating a data directory is logically independent of
> system call tracing.
You're right, I'll send another patch (i.e., another thread) in order to
add these modifications.
Well, I think that's it. Thanks in advance for your attention.
Regards,
--
Sérgio Durigan Júnior
Linux on Power Toolchain - Software Engineer
Linux Technology Center - LTC
IBM Brazil