From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22503 invoked by alias); 25 Apr 2014 17:21:01 -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 22165 invoked by uid 89); 25 Apr 2014 17:20:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: usevmg20.ericsson.net Received: from usevmg20.ericsson.net (HELO usevmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Fri, 25 Apr 2014 17:20:54 +0000 Received: from EUSAAHC005.ericsson.se (Unknown_Domain [147.117.188.87]) by usevmg20.ericsson.net (Symantec Mail Security) with SMTP id 52.2F.05011.20B4A535; Fri, 25 Apr 2014 13:46:10 +0200 (CEST) Received: from [142.133.110.254] (147.117.188.8) by smtps-am.internal.ericsson.com (147.117.188.87) with Microsoft SMTP Server (TLS) id 14.3.174.1; Fri, 25 Apr 2014 13:20:49 -0400 Message-ID: <535A997C.90904@ericsson.com> Date: Fri, 25 Apr 2014 17:21:00 -0000 From: Simon Marchi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 MIME-Version: 1.0 To: Pedro Alves CC: Subject: Re: [PATCH] gdbserver: Get the pid from /proc when attaching to a non-initial lwp References: <1398187705-17237-1-git-send-email-simon.marchi@ericsson.com> <535A8684.6000405@redhat.com> In-Reply-To: <535A8684.6000405@redhat.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg00532.txt.bz2 On 14-04-25 12:00 PM, Pedro Alves wrote: > On 04/22/2014 06:28 PM, Simon Marchi wrote: >> gdbserver fails to attach to a second inferior that is multi-threaded. >> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=16255 >> >> For the second inferior (and so on), current_inferior is still set to the >> first inferior when this code is ran. As a result, non-initial lwps of >> the second inferior get assigned the pid of the first inferior. >> >> One solution could be to switch current_inferior temporarily. Another >> one (which I chose) is to go get the pid (tgid in the linux terminology) >> in /proc. > > Thanks Simon. After you poked me earlier about this, I had an idea > for a different solution, which I think ends up being more complete, > and I've coded it now. See the description in the patch below. > >> I augmented the gdb.server/ext-attach.exp test case to attach to two >> inferiors simultaneously and made the test program multi-threaded. > > Thanks. That's very useful. Let's use it, but make it independent of > gdbserver. There's really nothing gdbserver specific about attaching > to two inferiors. All (multi-process capable) targets should be > able to do this. > > Let me know what you think. > > --------- > From 1705e97537ea392af4e4c2dd0c6e0ec1136f2e39 Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Fri, 25 Apr 2014 15:23:44 +0100 > Subject: [PATCH] PR server/16255: gdbserver cannot attach to a second > inferior that is multi-threaded. > > On Linux, we need to explicitly ptrace attach to all lwps of a > process. Because GDB might not be connected yet when an attach is > requested, and thus it may not be possible to activate thread_db, as > that requires access to symbols (IOW, gdbserver --attach), a while ago > we make linux_attach loop over the lwps as listed by /proc/PID/task to > find the lwps to attach to. > > linux_attach_lwp_1 has: > > ... > if (initial) > /* If lwp is the tgid, we handle adding existing threads later. > Otherwise we just add lwp without bothering about any other > threads. */ > ptid = ptid_build (lwpid, lwpid, 0); > else > { > /* Note that extracting the pid from the current inferior is > safe, since we're always called in the context of the same > process as this new thread. */ > int pid = pid_of (current_inferior); > ptid = ptid_build (pid, lwpid, 0); > } > > That "safe" comment referred to linux_attach_lwp being called by > thread-db.c. But this was clearly missed when a new call to > linux_attach_lwp_1 was added to linux_attach. As a result, > current_inferior will be set to some random process, and non-initial > lwps of the second inferior get assigned the pid of the wrong > inferior. E.g., in the case of attaching to two inferiors, for the > second inferior (and so on), non-initial lwps of the second inferior > get assigned the pid of the first inferior. This doesn't trigger on > the first inferior, when current_inferior is NULL, add_thread switches > the current inferior to the newly added thread. > > Rather than making linux_attach switch current_inferior temporarily > (thus avoiding further reliance on global state), or making > linux_attach_lwp_1 get the tgid from /proc, which add extra syscalls, > and will be wrong in case of the user having originally attached > directly to a non-tgid lwp, and then that lwp spawning new clones (the > ptid.pid field of further new clones should be the same as the > original lwp's pid, which is not the tgid), we note that callers of > linux_attach_lwp/linux_attach_lwp_1 always have the right pid handy > already, so they can pass it down along with the lwpid. > > The only other reason for the "initial" parameter is to error out > instead of warn in case of attach failure, when we're first attaching > to a process. There are only three callers of > linux_attach_lwp/linux_attach_lwp_1, and each wants to print a > different warn/error string, so we can just move the error/warn out of > linux_attach_lwp_1 to the callers, thus getting rid of the "initial" > parameter. > > There really nothing gdbserver-specific about attaching to two > threaded processes, so this adds a new test under gdb.multi/. The > test passes cleanly against the native GNU/Linux target, but > fails/triggers the bug against GDBserver (before the patch), with the > native-extended-remote board (as plain remote doesn't support > multi-process). > > Tested on x86_64 Fedora 17, with the native-extended-gdbserver board. > > gdb/gdbserver/ > 2014-04-25 Pedro Alves > > PR server/16255 > * linux-low.c (linux_attach_fail_reason_string): New function. > (linux_attach_lwp): Delete. > (linux_attach_lwp_1): Rename to ... > (linux_attach_lwp): ... this. Take a ptid instead of a pid as > argument. Remove "initial" parameter. Return int instead of > void. Don't error or warn here. > (linux_attach): Adjust to call linux_attach_lwp. Call error on > failure to attach to the tgid. Call warning when failing to > attach to an lwp. > * linux-low.h (linux_attach_lwp): Take a ptid instead of a pid as > argument. Remove "initial" parameter. Return int instead of > void. Don't error or warn here. > (linux_attach_fail_reason_string): New declaration. > * thread-db.c (attach_thread): Adjust to linux_attach_lwp's > interface change. Use linux_attach_fail_reason_string. > > gdb/ > 2014-04-25 Pedro Alves > > PR server/16255 > * common/linux-ptrace.c (linux_ptrace_attach_warnings): Rename to ... > (linux_ptrace_attach_fail_reason): ... this. Remove "warning: " > and newline from built string. > * common/linux-ptrace.h (linux_ptrace_attach_warnings): Rename to ... > (linux_ptrace_attach_fail_reason): ... this. > * linux-nat.c (linux_nat_attach): Adjust to use > linux_ptrace_attach_fail_reason. > > gdb/testsuite/ > 2014-04-25 Simon Marchi > Pedro Alves > > PR server/16255 > * gdb.multi/multi-attach.c: New file. > * gdb.multi/multi-attach.exp: New file. > --- > gdb/common/linux-ptrace.c | 16 ++--- > gdb/common/linux-ptrace.h | 2 +- > gdb/gdbserver/linux-low.c | 105 ++++++++++++++++--------------- > gdb/gdbserver/linux-low.h | 11 +++- > gdb/gdbserver/thread-db.c | 18 ++++-- > gdb/linux-nat.c | 7 ++- > gdb/testsuite/gdb.multi/multi-attach.c | 48 ++++++++++++++ > gdb/testsuite/gdb.multi/multi-attach.exp | 60 ++++++++++++++++++ > 8 files changed, 199 insertions(+), 68 deletions(-) > create mode 100644 gdb/testsuite/gdb.multi/multi-attach.c > create mode 100644 gdb/testsuite/gdb.multi/multi-attach.exp > > diff --git a/gdb/common/linux-ptrace.c b/gdb/common/linux-ptrace.c > index 7c1b78a..efbd1ea 100644 > --- a/gdb/common/linux-ptrace.c > +++ b/gdb/common/linux-ptrace.c > @@ -37,24 +37,24 @@ > 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. */ > +/* Find all possible reasons we could fail to attach PID and append > + these as strings to the already initialized BUFFER. '\0' > + termination of BUFFER must be done by the caller. */ > > void > -linux_ptrace_attach_warnings (pid_t pid, struct buffer *buffer) > +linux_ptrace_attach_fail_reason (pid_t pid, struct buffer *buffer) > { > pid_t tracerpid; > > tracerpid = linux_proc_get_tracerpid (pid); > if (tracerpid > 0) > - buffer_xml_printf (buffer, _("warning: process %d is already traced " > - "by process %d\n"), > + buffer_xml_printf (buffer, _("process %d is already traced " > + "by process %d"), > (int) pid, (int) tracerpid); > > if (linux_proc_pid_is_zombie (pid)) > - buffer_xml_printf (buffer, _("warning: process %d is a zombie " > - "- the process has already terminated\n"), > + buffer_xml_printf (buffer, _("process %d is a zombie " > + "- the process has already terminated"), > (int) pid); > } > > diff --git a/gdb/common/linux-ptrace.h b/gdb/common/linux-ptrace.h > index 38bb9ea..881d9c9 100644 > --- a/gdb/common/linux-ptrace.h > +++ b/gdb/common/linux-ptrace.h > @@ -83,7 +83,7 @@ struct buffer; > #define __WALL 0x40000000 /* Wait for any child. */ > #endif > > -extern void linux_ptrace_attach_warnings (pid_t pid, struct buffer *buffer); > +extern void linux_ptrace_attach_fail_reason (pid_t pid, struct buffer *buffer); > extern void linux_ptrace_init_warnings (void); > extern void linux_enable_event_reporting (pid_t pid); > extern int linux_supports_tracefork (void); > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index c847c62..0ef8d60 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -635,49 +635,41 @@ linux_create_inferior (char *program, char **allargs) > return pid; > } > > +char * > +linux_attach_fail_reason_string (ptid_t ptid, int err) > +{ > + static char *reason_string; > + struct buffer buffer; > + char *warnings; > + long lwpid = ptid_get_lwp (ptid); > + > + xfree (reason_string); > + > + buffer_init (&buffer); > + linux_ptrace_attach_fail_reason (lwpid, &buffer); > + buffer_grow_str0 (&buffer, ""); > + warnings = buffer_finish (&buffer); > + if (warnings[0] != '\0') > + reason_string = xstrprintf ("%s (%d), %s", > + strerror (err), err, warnings); > + else > + reason_string = xstrprintf ("%s (%d)", > + strerror (err), err); > + xfree (warnings); > + return reason_string; > +} > + > /* Attach to an inferior process. */ > > -static void > -linux_attach_lwp_1 (unsigned long lwpid, int initial) > +int > +linux_attach_lwp (ptid_t ptid) > { > - ptid_t ptid; > struct lwp_info *new_lwp; > + int lwpid = ptid_get_lwp (ptid); > > if (ptrace (PTRACE_ATTACH, lwpid, (PTRACE_TYPE_ARG3) 0, (PTRACE_TYPE_ARG4) 0) > != 0) > - { > - struct buffer buffer; > - > - if (!initial) > - { > - /* If we fail to attach to an LWP, just warn. */ > - fprintf (stderr, "Cannot attach to lwp %ld: %s (%d)\n", lwpid, > - strerror (errno), errno); > - fflush (stderr); > - return; > - } > - > - /* If we fail to attach to a process, report an error. */ > - buffer_init (&buffer); > - linux_ptrace_attach_warnings (lwpid, &buffer); > - buffer_grow_str0 (&buffer, ""); > - error ("%sCannot attach to lwp %ld: %s (%d)", buffer_finish (&buffer), > - lwpid, strerror (errno), errno); > - } > - > - if (initial) > - /* If lwp is the tgid, we handle adding existing threads later. > - Otherwise we just add lwp without bothering about any other > - threads. */ > - ptid = ptid_build (lwpid, lwpid, 0); > - else > - { > - /* Note that extracting the pid from the current inferior is > - safe, since we're always called in the context of the same > - process as this new thread. */ > - int pid = pid_of (current_inferior); > - ptid = ptid_build (pid, lwpid, 0); > - } > + return errno; > > new_lwp = add_lwp (ptid); > > @@ -747,12 +739,8 @@ linux_attach_lwp_1 (unsigned long lwpid, int initial) > end of the list, and so the new thread has not yet reached > wait_for_sigstop (but will). */ > new_lwp->stop_expected = 1; > -} > > -void > -linux_attach_lwp (unsigned long lwpid) > -{ > - linux_attach_lwp_1 (lwpid, 0); > + return 0; > } > > /* Attach to PID. If PID is the tgid, attach to it and all > @@ -761,9 +749,16 @@ linux_attach_lwp (unsigned long lwpid) > static int > linux_attach (unsigned long pid) > { > + ptid_t ptid = ptid_build (pid, pid, 0); > + int err; > + > /* Attach to PID. We will check for other threads > soon. */ > - linux_attach_lwp_1 (pid, 1); > + err = linux_attach_lwp (ptid); > + if (err != 0) > + error ("Cannot attach to process %ld: %s", > + pid, linux_attach_fail_reason_string (ptid, err)); > + > linux_add_process (pid, 1); > > if (!non_stop) > @@ -794,13 +789,13 @@ linux_attach (unsigned long pid) > { > /* At this point we attached to the tgid. Scan the task for > existing threads. */ > - unsigned long lwp; > int new_threads_found; > int iterations = 0; > - struct dirent *dp; > > while (iterations < 2) > { > + struct dirent *dp; > + > new_threads_found = 0; > /* Add all the other threads. While we go through the > threads, new threads may be spawned. Cycle through > @@ -808,19 +803,29 @@ linux_attach (unsigned long pid) > finding new threads. */ > while ((dp = readdir (dir)) != NULL) > { > + unsigned long lwp; > + ptid_t ptid; > + > /* Fetch one lwp. */ > lwp = strtoul (dp->d_name, NULL, 10); > > + ptid = ptid_build (pid, lwp, 0); > + > /* Is this a new thread? */ > - if (lwp > - && find_thread_ptid (ptid_build (pid, lwp, 0)) == NULL) > + if (lwp != 0 && find_thread_ptid (ptid) == NULL) > { > - linux_attach_lwp_1 (lwp, 0); > - new_threads_found++; > + int err; > > if (debug_threads) > - debug_printf ("Found and attached to new lwp %ld\n", > - lwp); > + debug_printf ("Found new lwp %ld\n", lwp); > + > + err = linux_attach_lwp (ptid); > + if (err != 0) > + warning ("Cannot attach to lwp %ld: %s", > + lwp, > + linux_attach_fail_reason_string (ptid, err)); > + > + new_threads_found++; > } > } > > diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h > index 7459710..cd3001d 100644 > --- a/gdb/gdbserver/linux-low.h > +++ b/gdb/gdbserver/linux-low.h > @@ -343,7 +343,16 @@ struct lwp_info > > int linux_pid_exe_is_elf_64_file (int pid, unsigned int *machine); > > -void linux_attach_lwp (unsigned long pid); > +/* Attach to PTID. Returns 0 on success, non-zero otherwise (an > + errno). */ > +int linux_attach_lwp (ptid_t ptid); > + > +/* Return the reason an attach failed, in string form. ERR is the > + error returned by linux_attach_lwp (an errno). This string should > + be copied into a buffer by the client if the string will not be > + immediately used, or if it must persist. */ > +char *linux_attach_fail_reason_string (ptid_t ptid, int err); > + > struct lwp_info *find_lwp_pid (ptid_t ptid); > void linux_stop_lwp (struct lwp_info *lwp); > > diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c > index f63e39e..ae0d191 100644 > --- a/gdb/gdbserver/thread-db.c > +++ b/gdb/gdbserver/thread-db.c > @@ -323,27 +323,33 @@ find_one_thread (ptid_t ptid) > static int > attach_thread (const td_thrhandle_t *th_p, td_thrinfo_t *ti_p) > { > + struct process_info *proc = current_process (); > + int pid = pid_of (proc); > + ptid_t ptid = ptid_build (pid, ti_p->ti_lid, 0); > struct lwp_info *lwp; > + int err; > > if (debug_threads) > debug_printf ("Attaching to thread %ld (LWP %d)\n", > ti_p->ti_tid, ti_p->ti_lid); > - linux_attach_lwp (ti_p->ti_lid); > - lwp = find_lwp_pid (pid_to_ptid (ti_p->ti_lid)); > - if (lwp == NULL) > + err = linux_attach_lwp (ptid); > + if (err != 0) > { > - warning ("Could not attach to thread %ld (LWP %d)\n", > - ti_p->ti_tid, ti_p->ti_lid); > + warning ("Could not attach to thread %ld (LWP %d): %s\n", > + ti_p->ti_tid, ti_p->ti_lid, > + linux_attach_fail_reason_string (ptid, err)); > return 0; > } > > + lwp = find_lwp_pid (ptid); > + gdb_assert (lwp != NULL); > lwp->thread_known = 1; > lwp->th = *th_p; > > if (thread_db_use_events) > { > td_err_e err; > - struct thread_db *thread_db = current_process ()->private->thread_db; > + struct thread_db *thread_db = proc->private->thread_db; > > err = thread_db->td_thr_event_enable_p (th_p, 1); > if (err != TD_OK) > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c > index d08cb13..e84ee95 100644 > --- a/gdb/linux-nat.c > +++ b/gdb/linux-nat.c > @@ -1320,13 +1320,16 @@ linux_nat_attach (struct target_ops *ops, char *args, int from_tty) > make_cleanup (xfree, message); > > buffer_init (&buffer); > - linux_ptrace_attach_warnings (pid, &buffer); > + linux_ptrace_attach_fail_reason (pid, &buffer); > > buffer_grow_str0 (&buffer, ""); > buffer_s = buffer_finish (&buffer); > make_cleanup (xfree, buffer_s); > > - throw_error (ex.error, "%s%s", buffer_s, message); > + if (*buffer_s != '\0') > + throw_error (ex.error, "warning: %s\n%s", buffer_s, message); > + else > + throw_error (ex.error, "%s", message); > } > > /* The ptrace base target adds the main thread with (pid,0,0) > diff --git a/gdb/testsuite/gdb.multi/multi-attach.c b/gdb/testsuite/gdb.multi/multi-attach.c > new file mode 100644 > index 0000000..0fb0a64 > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/multi-attach.c > @@ -0,0 +1,48 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2007-2014 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +/* This program is intended to be started outside of gdb, and then > + attached to by gdb. It loops for a while, but not forever. */ > + > +#include > +#include > + > +static void * > +thread_func (void *arg) > +{ > + int i; > + > + for (i = 0; i < 120; i++) > + sleep (1); > + > + return NULL; > +} > + > +int main () > +{ > + int i; > + pthread_t thread; > + > + pthread_create (&thread, NULL, thread_func, NULL); > + > + for (i = 0; i < 120; i++) > + sleep (1); > + > + pthread_join (thread, NULL); > + > + return 0; > +} > diff --git a/gdb/testsuite/gdb.multi/multi-attach.exp b/gdb/testsuite/gdb.multi/multi-attach.exp > new file mode 100644 > index 0000000..e933520 > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/multi-attach.exp > @@ -0,0 +1,60 @@ > +# This testcase is part of GDB, the GNU debugger. > + > +# Copyright 2007-2014 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# Test attaching to multiple threaded programs. > + > +standard_testfile > + > +# We need to use TCL's exec to get the pid. > +if [is_remote target] then { > + return 0 > +} > + > +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug additional_flags=-lpthread}]} { > + return -1 > +} > + > +# Start the programs running and then wait for a bit, to be sure that > +# they can be attached to. > +set testpid1 [eval exec $binfile &] > +set testpid2 [eval exec $binfile &] > +exec sleep 2 > +if { [istarget "*-*-cygwin*"] } { > + # testpid{1,2} are the Cygwin PID, GDB uses the Windows PID, which might be > + # different due to the way fork/exec works. > + set testpid1 [ exec ps -e | gawk "{ if (\$1 == $testpid1) print \$4; }" ] > + set testpid2 [ exec ps -e | gawk "{ if (\$1 == $testpid2) print \$4; }" ] > +} > + > +gdb_test "attach $testpid1" \ > + "Attaching to program: .*, process $testpid1.*(in|at).*" \ > + "attach to program 1" > +gdb_test "backtrace" ".*main.*" "backtrace 1" > + > +gdb_test "add-inferior -exec $binfile" \ > + "Added inferior 2.*" \ > + "add second inferior" > +gdb_test "inferior 2" ".*Switching to inferior 2.*" "switch to second inferior" > + > +gdb_test "attach $testpid2" \ > + "Attaching to program: .*, process $testpid2.*(in|at).*" \ > + "attach to program 2" > +gdb_test "backtrace" ".*main.*" "backtrace 2" > + > +gdb_test "kill" "" "kill inferior 2" "Kill the program being debugged.*" "y" > +gdb_test "inferior 1" ".*Switching to inferior 1.*" > +gdb_test "kill" "" "kill inferior 1" "Kill the program being debugged.*" "y" All of this makes sense. I tested your change with my previously failing test case it works fine. I considered this solution (passing the pid from linux_attach), but I was a bit puzzled with what to do with these other calls. Thanks, Simon