This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] gdbserver: Get the pid from /proc when attaching to a non-initial lwp
- From: Simon Marchi <simon dot marchi at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Fri, 25 Apr 2014 13:21:00 -0400
- Subject: Re: [PATCH] gdbserver: Get the pid from /proc when attaching to a non-initial lwp
- Authentication-results: sourceware.org; auth=none
- References: <1398187705-17237-1-git-send-email-simon dot marchi at ericsson dot com> <535A8684 dot 6000405 at redhat dot com>
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 <palves@redhat.com>
> 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 <palves@redhat.com>
>
> 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 <palves@redhat.com>
>
> 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 <simon.marchi@ericsson.com>
> Pedro Alves <palves@redhat.com>
>
> 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 <http://www.gnu.org/licenses/>. */
> +
> +/* 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 <unistd.h>
> +#include <pthread.h>
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +# 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