From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1034 invoked by alias); 25 Feb 2009 10:26:54 -0000 Received: (qmail 1026 invoked by uid 22791); 25 Feb 2009 10:26:52 -0000 X-SWARE-Spam-Status: No, hits=-2.7 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from e24smtp03.br.ibm.com (HELO e24smtp03.br.ibm.com) (32.104.18.24) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 25 Feb 2009 10:26:46 +0000 Received: from d24relay01.br.ibm.com (d24relay01.br.ibm.com [9.8.31.16]) by e24smtp03.br.ibm.com (8.13.1/8.13.1) with ESMTP id n1PAOJw0031765 for ; Wed, 25 Feb 2009 07:24:19 -0300 Received: from d24av01.br.ibm.com (d24av01.br.ibm.com [9.18.232.46]) by d24relay01.br.ibm.com (8.13.8/8.13.8/NCO v9.2) with ESMTP id n1PBQ6p03961048 for ; Wed, 25 Feb 2009 08:26:06 -0300 Received: from d24av01.br.ibm.com (loopback [127.0.0.1]) by d24av01.br.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id n1PAQeEi011677 for ; Wed, 25 Feb 2009 07:26:41 -0300 Received: from [9.8.5.92] ([9.8.5.92]) by d24av01.br.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id n1PAQe2k011633; Wed, 25 Feb 2009 07:26:40 -0300 Subject: Re: [PATCH 1/4] catch syscall -- try 4 -- Architecture-independent part From: =?ISO-8859-1?Q?S=E9rgio?= Durigan =?ISO-8859-1?Q?J=FAnior?= To: Daniel Jacobowitz Cc: Pedro Alves , gdb-patches@sourceware.org, teawater In-Reply-To: <20090201193306.GJ4597@caradoc.them.org> 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> Content-Type: text/plain; charset=iso-8859-1 Date: Wed, 25 Feb 2009 21:44:00 -0000 Message-Id: <1235561189.14363.20.camel@miki> Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2009-02/txt/msg00481.txt.bz2 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