From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17947 invoked by alias); 20 Aug 2013 16:39:30 -0000 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 Received: (qmail 17923 invoked by uid 89); 20 Aug 2013 16:39:30 -0000 X-Spam-SWARE-Status: No, score=-8.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 20 Aug 2013 16:39:28 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7KGdQ7t001618 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 20 Aug 2013 12:39:26 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r7KGdN0l031243; Tue, 20 Aug 2013 12:39:24 -0400 Message-ID: <52139BBA.60300@redhat.com> Date: Tue, 20 Aug 2013 16:39:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: lgustavo@codesourcery.com CC: "'gdb-patches@sourceware.org'" , Tom Tromey Subject: Re: [PATCH, v2] Share ptrace options discovery/linux native code between GDB and gdbserver References: <5212A9E1.6030707@codesourcery.com> In-Reply-To: <5212A9E1.6030707@codesourcery.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-08/txt/msg00548.txt.bz2 On 08/20/2013 12:27 AM, Luis Machado wrote: > diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c > index d5ac061..4198d1d 100644 > --- a/gdb/common/linux-ptrace.c > +++ b/gdb/common/linux-ptrace.c > @@ -25,10 +25,16 @@ > > #include "linux-ptrace.h" > #include "linux-procfs.h" > +#include "nat/linux-waitpid.h" > #include "buffer.h" > #include "gdb_assert.h" > #include "gdb_wait.h" > > +/* Stores the currently supported ptrace options. A value of > + -1 means we did not check for features yet. A value of 0 means > + there are no supported features. */ > +static int current_ptrace_options = -1; > + > /* Find all possible reasons we could fail to attach PID and append these > newline terminated reason strings to initialized BUFFER. '\0' termination > of BUFFER must be done by the caller. */ > @@ -222,6 +228,284 @@ linux_ptrace_test_ret_to_nx (void) > #endif /* defined __i386__ || defined __x86_64__ */ > } > > +/* Helper function to fork a process and make the child process call > + the function FUNCTION passing ARG as parameter. */ > + > +static int > +linux_fork_to_function (void *arg, void (*function) (void *)) The describing comment and the function prototype imply some sort of generity. But ... > +{ > + int child_pid; > + > + gdb_byte *stack = (gdb_byte *) arg; The arg has a definite particular use. > + > + /* Sanity check the function pointer. */ > + gdb_assert (function != NULL); > + > +#if defined(__UCLIBC__) && defined(HAS_NOMMU) > +#define STACK_SIZE 4096 > + > + if (arg == NULL) > + stack = xmalloc (STACK_SIZE * 4); Etc. Something's odd with the abstration. > + > + /* Use CLONE_VM instead of fork, to support uClinux (no MMU). */ > + #ifdef __ia64__ > + child_pid = __clone2 (function, stack, STACK_SIZE, > + CLONE_VM | SIGCHLD, stack + STACK_SIZE * 2); > + #else /* !__ia64__ */ > + child_pid = clone (function, stack + STACK_SIZE, > + CLONE_VM | SIGCHLD, stack + STACK_SIZE * 2); > + #endif /* !__ia64__ */ > +#else /* !defined(__UCLIBC) && defined(HAS_NOMMU) */ > + child_pid = fork (); > + > + if (child_pid == 0) > + function (stack); > +#endif /* defined(__UCLIBC) && defined(HAS_NOMMU) */ > + > + if (child_pid == -1) > + perror_with_name (("fork")); > + > + return child_pid; > +} > + > + > +/* Determine ptrace features available on this target. */ > + > +static void > +linux_check_ptrace_features (void) > +{ > + int child_pid, ret, status; > + long second_pid; > + > + /* Initialize the options. */ > + current_ptrace_options = 0; > + > + /* Fork a child so we can do some testing. The child will call > + linux_child_function and will get traced. The child will > + eventually fork a grandchild so we can test fork event > + reporting. */ > + child_pid = linux_fork_to_function (NULL, linux_child_function); > + > + ret = my_waitpid (child_pid, &status, 0); > + if (ret == -1) > + perror_with_name (("waitpid")); > + else if (ret != child_pid) > + error (_("linux_check_ptrace_features: waitpid: unexpected result %d."), > + ret); > + if (! WIFSTOPPED (status)) > + error (_("linux_check_ptrace_features: waitpid: unexpected status %d."), > + status); > + > +#ifdef GDBSERVER > + /* gdbserver does not support PTRACE_O_TRACEFORK yet. */ This here doesn't look right ... > +#else > + /* First, set the PTRACE_O_TRACEFORK option. If this fails, we > + know for sure that it is not supported. */ > + ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0, > + PTRACE_O_TRACEFORK); > +#endif > + > + if (ret != 0) ... as this looks like will always be reached for gdbserver. The PTRACE_O_TRACEFORK testing is being used in gdbserver as proxy for close tracing support. > + { > + ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0, > + (PTRACE_TYPE_ARG4) 0); > + if (ret != 0) > + { > + warning (_("linux_check_ptrace_features: failed to kill child")); > + return; > + } > + > + ret = my_waitpid (child_pid, &status, 0); > + if (ret != child_pid) > + warning (_("linux_check_ptrace_features: failed " > + "to wait for killed child")); > + else if (!WIFSIGNALED (status)) > + warning (_("linux_check_ptrace_features: unexpected " > + "wait status 0x%x from killed child"), status); > + > + return; > + } > + > +#ifdef GDBSERVER > + /* gdbserver does not support PTRACE_O_TRACESYSGOOD or > + PTRACE_O_TRACEVFORKDONE yet. */ > +#else > + /* Check if the target supports PTRACE_O_TRACESYSGOOD. */ > + ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0, > + PTRACE_O_TRACESYSGOOD); > + current_ptrace_options |= (ret == 0)? PTRACE_O_TRACESYSGOOD : 0; Space before '?'. Arguably, if (ret == 0) current_ptrace_options |= PTRACE_O_TRACESYSGOOD; would be more straightforward to read. > + > + /* Check if the target supports PTRACE_O_TRACEVFORKDONE. */ > + ret = ptrace (PTRACE_SETOPTIONS, child_pid, (PTRACE_TYPE_ARG3) 0, > + PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORKDONE); > + current_ptrace_options |= (ret == 0)? PTRACE_O_TRACEVFORKDONE : 0; Ditto missing space. > +#endif > + > + /* Setting PTRACE_O_TRACEFORK did not cause an error, however we > + don't know for sure that the feature is available; old > + versions of PTRACE_SETOPTIONS ignored unknown options. > + Therefore, we attach to the child process, use PTRACE_SETOPTIONS > + to enable fork tracing, and let it fork. If the process exits, > + we assume that we can't use PTRACE_O_TRACEFORK; if we get the > + fork notification, and we can extract the new child's PID, then > + we assume that we can. > + > + We do not explicitly check for vfork tracing here. It is > + assumed that vfork tracing is available whenever fork tracing > + is available. */ > + ret = ptrace (PTRACE_CONT, child_pid, (PTRACE_TYPE_ARG3) 0, > + (PTRACE_TYPE_ARG4) 0); > + if (ret != 0) > + warning (_("linux_check_ptrace_features: failed to resume child")); > + > + ret = my_waitpid (child_pid, &status, 0); > + > + /* Check if we received a fork event notification. */ > + if (ret == child_pid && WIFSTOPPED (status) > + && status >> 16 == PTRACE_EVENT_FORK) > + { > + /* We did receive a fork event notification. Make sure its PID > + is reported. */ > + second_pid = 0; > + ret = ptrace (PTRACE_GETEVENTMSG, child_pid, (PTRACE_TYPE_ARG3) 0, > + (PTRACE_TYPE_ARG4) &second_pid); > + if (ret == 0 && second_pid != 0) > + { > + int second_status; > + > + /* We got the PID from the grandchild, which means fork > + tracing is supported. */ > +#ifdef GDBSERVER > + /* Do not enable all the options for now since gdbserver does not > + properly support them. This restriction will be lifted when > + gdbserver is augmented to support them. */ > + current_ptrace_options |= PTRACE_O_TRACECLONE; > +#else > + current_ptrace_options |= PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK > + | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC; > +#endif > + > + /* Do some cleanup and kill the grandchild. */ > + my_waitpid (second_pid, &second_status, 0); > + ret = ptrace (PTRACE_KILL, second_pid, (PTRACE_TYPE_ARG3) 0, > + (PTRACE_TYPE_ARG4) 0); > + if (ret != 0) > + warning (_("linux_check_ptrace_features: " > + "failed to kill second child")); > + my_waitpid (second_pid, &status, 0); > + } > + } > + else > + warning (_("linux_check_ptrace_features: unexpected result from waitpid " > + "(%d, status 0x%x)"), ret, status); > + > + /* Clean things up and kill any pending childs. */ s/childs/children/. > + do > + { > + ret = ptrace (PTRACE_KILL, child_pid, (PTRACE_TYPE_ARG3) 0, > + (PTRACE_TYPE_ARG4) 0); > + if (ret != 0) > + warning ("linux_check_ptrace_features: failed to kill child"); > + my_waitpid (child_pid, &status, 0); > + } > + while (WIFSTOPPED (status)); > +} > + > +/* Enable reporting of all currently supported ptrace events. */ > + > +void > +linux_enable_event_reporting (ptid_t ptid) Could you preserve gdbserver's prototype here, please? That is, take a single integer pid rather than a ptid. > +{ > + int pid = ptid_get_lwp (ptid); > + > + if (pid == 0) > + pid = ptid_get_pid (ptid); This dance is only really necessary for GDB. Given this is working at the ptrace level, passing in the pid only is more to the point. (E.g., passing in a ptid makes the reader wonder whether PTID can be represent a whole inferior, and thus event reporting enablement gets applied to all threads). > + > + /* Check if we have initialized the ptrace features for this > + target. If not, do it now. */ > + if (current_ptrace_options == -1) > + linux_check_ptrace_features (); > + > + /* Do not enable PTRACE_O_TRACEEXIT until GDB is more prepared to > + support read-only process state. */ This comment really belongs close to where current_ptrace_options is set. That in, in the original code, read it as attached to the "current_ptrace_options |= " bits above, not the ptrace call below. > + > + /* Set the options. */ > + ptrace (PTRACE_SETOPTIONS, pid, (PTRACE_TYPE_ARG3) 0, > + current_ptrace_options); > +} > + > +/* Returns non-zero if PTRACE_OPTIONS is contained within > + CURRENT_PTRACE_OPTIONS, therefore supported. Returns 0 > + otherwise. */ > + > +static int > +ptrace_supports_feature (int ptrace_options) > +{ > + gdb_assert (current_ptrace_options >= 0); > + > + return ((current_ptrace_options & ptrace_options) == ptrace_options); > +} > + > +/* Returns non-zero if PTRACE_O_TRACEFORK, PTRACE_O_TRACEVFORK, > + PTRACE_O_TRACECLONE and PTRACE_O_TRACEEXEC are supported by > + ptrace, 0 otherwise */ Missing period. Given the name of the function, I'd suggest instead: /* Returns non-zero if PTRACE_EVENT_FORK is supported by ptrace, 0 otherwise. Note that if PTRACE_EVENT_FORK is supported so is PTRACE_EVENT_CLONE, PTRACE_EVENT_EXEC and PTRACE_EVENT_VFORK, since they were all added to the kernel at the same time. > + > +int > +linux_supports_tracefork (void) > +{ > + return ptrace_supports_feature (PTRACE_O_TRACEFORK | PTRACE_O_TRACEVFORK > + | PTRACE_O_TRACECLONE | PTRACE_O_TRACEEXEC); Well, this is now wrong given gdbserver will only set PTRACE_O_TRACECLONE in the current_ptrace_options. Given the revised comment above, this can all be replaced with: return ptrace_supports_feature (PTRACE_O_TRACECLONE); and so it'll work for both gdb and gdbserver. When gdbserver supports tracing forks, we can go back here and check PTRACE_O_TRACEFORK instead, just for clarity of the code. WDYT? BTW, these function are extern, and the same describing comment is duplicated in the declarations in the header file. Best leave only one copy, near the declarations. > +} > + > +/* Returns non-zero if PTRACE_O_TRACEVFORKDONE is supported by > + ptrace, 0 otherwise */ > + > +int > +linux_supports_tracevforkdone (void) > +{ > + return ptrace_supports_feature (PTRACE_O_TRACEVFORKDONE); > +} > + > +/* Returns non-zero if PTRACE_O_TRACESYSGOOD is supported by ptrace, > + 0 otherwise */ > + > +int > +linux_supports_tracesysgood (void) > +{ > + return ptrace_supports_feature (PTRACE_O_TRACESYSGOOD); > +} > + > /* Display possible problems on this system. Display them only once per GDB > execution. */ > > diff --git a/gdb/common/linux-ptrace.h b/gdb/common/linux-ptrace.h > index 8f02c82..005da3d 100644 > --- a/gdb/common/linux-ptrace.h > +++ b/gdb/common/linux-ptrace.h > @@ -22,6 +22,24 @@ struct buffer; > > +#ifdef GDBSERVER > +#if !defined (PTRACE_TYPE_ARG3) Why the #if !defines if the PTRACE_TYPE_... definitions have been removed from linux-low.h ? > +#define PTRACE_TYPE_ARG3 void * > +#endif > + > +#if !defined (PTRACE_TYPE_ARG4) > +#define PTRACE_TYPE_ARG4 void * > +#endif > +#endif /* GDBSERVER */ > + > #ifndef PTRACE_GETSIGINFO > # define PTRACE_GETSIGINFO 0x4202 > # define PTRACE_SETSIGINFO 0x4203 > @@ -70,4 +88,19 @@ struct buffer; > extern void linux_ptrace_attach_warnings (pid_t pid, struct buffer *buffer); > extern void linux_ptrace_init_warnings (void); > > +/* Enable reporting of all currently supported ptrace events. */ > +extern void linux_enable_event_reporting (ptid_t ptid); > + > +/* Returns non-zero if PTRACE_EVENT_FORK, PTRACE_EVENT_CLONE, PTRACE_EVENT_EXEC > + and PTRACE_EVENT_VFORK are supported by ptrace, 0 otherwise */ > +extern int linux_supports_tracefork (void); > + > +/* Returns non-zero if PTRACE_O_TRACEVFORKDONE is supported by ptrace, > + 0 otherwise */ > +extern int linux_supports_tracevforkdone (void); > + > +/* Returns non-zero if PTRACE_O_TRACESYSGOOD is supported by ptrace, > + 0 otherwise */ > +extern int linux_supports_tracesysgood (void); > + > #endif /* COMMON_LINUX_PTRACE_H */ > diff --git a/gdb/config.in b/gdb/config.in > index 76abd04..5a80001 100644 > --- a/gdb/config.in > +++ b/gdb/config.in > @@ -659,6 +659,9 @@ > /* Define to the type of arg 3 for ptrace. */ > #undef PTRACE_TYPE_ARG3 > > +/* Define to the type of arg 3 for ptrace. */ > +#undef PTRACE_TYPE_ARG4 Off by one in comment... > + > /* Define to the type of arg 5 for ptrace. */ > #undef PTRACE_TYPE_ARG5 > > diff --git a/gdb/configure.ac b/gdb/configure.ac > index 667821f..0982cac 100644 > --- a/gdb/configure.ac > +++ b/gdb/configure.ac > @@ -1207,7 +1207,7 @@ AC_CACHE_CHECK([types of arguments for ptrace], gdb_cv_func_ptrace_args, [ > for gdb_arg1 in 'int' 'long'; do > for gdb_arg2 in 'pid_t' 'int' 'long'; do > for gdb_arg3 in 'int *' 'caddr_t' 'int' 'long' 'void *'; do > - for gdb_arg4 in 'int' 'long'; do > + for gdb_arg4 in 'int' 'long' 'void *'; do > AC_TRY_COMPILE($gdb_ptrace_headers, [ > extern $gdb_cv_func_ptrace_ret > ptrace ($gdb_arg1, $gdb_arg2, $gdb_arg3, $gdb_arg4); > @@ -1234,6 +1234,8 @@ IFS=$ac_save_IFS > shift > AC_DEFINE_UNQUOTED(PTRACE_TYPE_ARG3, $[3], > [Define to the type of arg 3 for ptrace.]) > +AC_DEFINE_UNQUOTED(PTRACE_TYPE_ARG4, $[4], > + [Define to the type of arg 4 for ptrace.]) ... but here the comment looks right, so I think you just forgot to regenerate config.in. > if test -n "$[5]"; then > AC_DEFINE_UNQUOTED(PTRACE_TYPE_ARG5, $[5], > [Define to the type of arg 5 for ptrace.]) > diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in > index 2cdbf47..04c061a 100644 > --- a/gdb/gdbserver/Makefile.in > +++ b/gdb/gdbserver/Makefile.in > @@ -100,6 +100,11 @@ GNULIB_H = $(GNULIB_BUILDDIR)/import/string.h @GNULIB_STDINT_H@ > # -I. for config files. > # -I${srcdir} for our headers. > # -I$(srcdir)/../regformats for regdef.h. > +# > +# We do not include ../target or ../nat in here because headers > +# in those directories should be included with the subdirectory. > +# e.g.: "target/wait.h > +# Missing final quote, and probably period: # E.g.: "target/wait.h". > INCLUDE_CFLAGS = -I. -I${srcdir} -I$(srcdir)/../common \ > -I$(srcdir)/../regformats -I$(srcdir)/../ -I$(INCLUDE_DIR) \ > $(INCGNU) > @@ -157,8 +162,10 @@ SFILES= $(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \ > $(srcdir)/common/common-utils.c $(srcdir)/common/xml-utils.c \ > $(srcdir)/common/linux-osdata.c $(srcdir)/common/ptid.c \ > $(srcdir)/common/buffer.c $(srcdir)/common/linux-btrace.c \ > - $(srcdir)/common/filestuff.c $(srcdir)/target/waitstatus.c \ > - $(srcdir)/common/mips-linux-watch.c > + $(srcdir)/common/filestuff.c $(srcdir)/common/linux-ptrace.c \ > + $(srcdir)/common/mips-linux-watch.c \ > + $(srcdir)/target/waitstatus.c \ > + $(srcdir)/nat/linux-waitpid.c \ Hmm, so linux-ptrace.c was missing? (It's a bit harder than expected to see what's doing on in this hunk, given the order's been changed.) It seems linux-procfs.c is missing too, and probably others. Please do that as separate patch... This is penance for forgetting that backslash in the last line, and forcing me to look closer. ;-) > > DEPFILES = @GDBSERVER_DEPFILES@ > > @@ -447,7 +454,8 @@ server_h = $(srcdir)/server.h $(regcache_h) $(srcdir)/target.h \ > $(generated_files) > > gdbthread_h = $(srcdir)/gdbthread.h $(target_h) $(srcdir)/server.h > -linux_low_h = $(srcdir)/linux-low.h $(gdbthread_h) > +linux_low_h = $(srcdir)/linux-low.h $(srcdir)/../nat/linux-nat.h \ > + $(srcdir)/../nat/linux-waitpid.h $(gdbthread_h) Is this really necessary, given we now have automatic dependencies in gdbserver? $linux_low_h even used anywhere at all. Looks like these variables are just waiting to be garbage collected... > > linux_ptrace_h = $(srcdir)/../common/linux-ptrace.h > > @@ -562,6 +570,12 @@ linux-btrace.o: ../common/linux-btrace.c $(linux_btrace_h) $(server_h) > mips-linux-watch.o: ../common/mips-linux-watch.c $(mips_linux_watch_h) $(server_h) > $(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< > > +# Shared native object files rules from ../nat > + > +linux-waitpid.o: ../nat/linux-waitpid.c > + $(COMPILE) $< > + $(POSTCOMPILE) > + > # We build vasprintf with -DHAVE_CONFIG_H because we want that unit to > # include our config.h file. Otherwise, some system headers do not get > # included, and the compiler emits a warning about implicitly defined > diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv > index b9dfd6c..b5b878c 100644 > --- a/gdb/gdbserver/configure.srv > +++ b/gdb/gdbserver/configure.srv > @@ -39,15 +39,18 @@ srv_amd64_xmlfiles="i386/amd64.xml i386/amd64-avx.xml i386/x32.xml i386/x32-avx. > srv_i386_linux_xmlfiles="i386/i386-linux.xml i386/i386-avx-linux.xml i386/i386-mmx-linux.xml i386/32bit-linux.xml $srv_i386_32bit_xmlfiles" > srv_amd64_linux_xmlfiles="i386/amd64-linux.xml i386/amd64-avx-linux.xml i386/64bit-linux.xml i386/x32-linux.xml i386/x32-avx-linux.xml $srv_i386_64bit_xmlfiles" > > + > +# Native linux object files. This is so we don't have to repeat > +# these files over and over again. > +srv_native_linux_obj="linux-waitpid.o linux-low.o linux-osdata.o linux-procfs.o" Let's just drop the "native" bit. All objects in this file are native: # Linux object files. This is so we don't have to repeat # these files over and over again. srv_linux_obj="linux-waitpid.o linux-low.o linux-osdata.o linux-procfs.o" Thanks, -- Pedro Alves