From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 6DB9E385B1A1 for ; Mon, 28 Nov 2022 13:31:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6DB9E385B1A1 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669642265; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3bvaCC7TO7kclkuiOET3i4dfc/dk2gFdGtNZEf9qqu0=; b=iV3T+ybIWE9sAG90FK24EDAS2k7zqeO5ar+lpax2AadnlAUB+1vU2j6MJPxjPzoh5oZLb7 Ibmzp983fbZntzrVkSIlz9BIJNKQUytJqoD+Bz6TehcitVnMrQSlLgvHWtMi9TLFOW74t0 ruheDkNxTq5OHaJuq2gXyAL5mohFAzg= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-348-UkF85KoCOReCvSuQAZjKkA-1; Mon, 28 Nov 2022 08:31:01 -0500 X-MC-Unique: UkF85KoCOReCvSuQAZjKkA-1 Received: by mail-wr1-f71.google.com with SMTP id r17-20020adfb1d1000000b002421ae7fd46so537163wra.10 for ; Mon, 28 Nov 2022 05:31:00 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:references :in-reply-to:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=QnQD3SUcdKJ2oyzVzoXMFL+G9xpJEqMQ5mAkUfdkGMU=; b=jZXz+aXzA44AkURPc4vGAQaxTsAd2YX5y7sB+VoLDTTOeLvBNbRAsMSczRZfeWEPu8 ht626m5qNKN1e1q4cEIN6EtIHt96ml5V9i4GPT/ktN5xlrcgolnSNT/XhzRjf5CU61Qx YrRomdCCgACZd9rKnA8VfBJ0LbCzTsQIiR2/iIHfv8vA+IcxNxmpcFI3MzQSKD+Y61oi ItvGB486lyKe1MOZIpaEMTJ4b+ULslKE/aFr8eirBwMMCgkFhEw2789BdlLdxXeytI8V gen35znHj5aA6gU4N7Y1cqI9fZ3uzXjATFoZlXlj9XWwopucf8MPx0XowkD6xPqPpXBZ gMhA== X-Gm-Message-State: ANoB5pk0mufrVeJMLJM+rwo8womlZrfrsgZ+ICVL6XQlG0rUmbcP9hD3 7evSMYZFNxZUmBdRBjz88Ow+sgUmnCqeAWp16HIgihDwQ30YE1Kr9LyaOOUXTLRzV6EDoyg7aJk qupVSJ6PIIDOWvutTA4N8pg== X-Received: by 2002:a05:600c:3d0c:b0:3cf:f66c:9246 with SMTP id bh12-20020a05600c3d0c00b003cff66c9246mr25097041wmb.27.1669642259595; Mon, 28 Nov 2022 05:30:59 -0800 (PST) X-Google-Smtp-Source: AA0mqf6gtlzqh+TLCJFKIQwVu9Xz8QaehJgwI+IzY2JaD2Opv65BHDDfXZ9OBf65JTNNqngPGVlXww== X-Received: by 2002:a05:600c:3d0c:b0:3cf:f66c:9246 with SMTP id bh12-20020a05600c3d0c00b003cff66c9246mr25096987wmb.27.1669642259131; Mon, 28 Nov 2022 05:30:59 -0800 (PST) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id s3-20020a5d4243000000b00241e4bff85asm10675252wrr.100.2022.11.28.05.30.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 05:30:58 -0800 (PST) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Cc: Simon Marchi Subject: Re: [PATCH v2 4/5] gdbserver: switch to right process in find_one_thread In-Reply-To: <20221121171213.1414366-5-simon.marchi@polymtl.ca> References: <20221121171213.1414366-1-simon.marchi@polymtl.ca> <20221121171213.1414366-5-simon.marchi@polymtl.ca> Date: Mon, 28 Nov 2022 13:30:57 +0000 Message-ID: <87cz972oku.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP,WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Simon Marchi via Gdb-patches writes: > From: Simon Marchi > > New in this version: add a dedicated test. > > When I do this: > > $ ./gdb -nx --data-directory=3Ddata-directory -q \ > /bin/sleep \ > =09-ex "maint set target-non-stop on" \ > =09-ex "tar ext :1234" \ > =09-ex "set remote exec-file /bin/sleep" \ > =09-ex "run 1231 &" \ > =09-ex add-inferior \ > =09-ex "inferior 2" > Reading symbols from /bin/sleep... > (No debugging symbols found in /bin/sleep) > Remote debugging using :1234 > Starting program: /bin/sleep 1231 > Reading /lib64/ld-linux-x86-64.so.2 from remote target... > warning: File transfers from remote targets can be slow. Use "set sys= root" to access files locally instead. > Reading /lib64/ld-linux-x86-64.so.2 from remote target... > Reading /usr/lib/debug/.build-id/a6/7a1408f18db3576757eea210d07ba3fc5= 60dff.debug from remote target... > [New inferior 2] > Added inferior 2 on connection 1 (extended-remote :1234) > [Switching to inferior 2 [] ()] > (gdb) Reading /lib/x86_64-linux-gnu/libc.so.6 from remote target... > attach 3659848 > Attaching to process 3659848 > /home/smarchi/src/binutils-gdb/gdb/thread.c:85: internal-error: infer= ior_thread: Assertion `current_thread_ !=3D nullptr' failed. > > Note the "attach" command just above. When doing it on the command-line > with a -ex switch, the bug doesn't trigger. > > The internal error of GDB is actually caused by GDBserver crashing, and > the error recovery of GDB is not on point. This patch aims to fix just > the GDBserver crash, not the GDB problem. > > GDBserver crashes with a segfault here: > > (gdb) bt > #0 0x00005555557fb3f4 in find_one_thread (ptid=3D...) at /home/smarc= hi/src/binutils-gdb/gdbserver/thread-db.cc:177 > #1 0x00005555557fd5cf in thread_db_thread_handle (ptid=3D, handle= =3D0x7fffffffc400, handle_len=3D0x7fffffffc3f0) > at /home/smarchi/src/binutils-gdb/gdbserver/thread-db.cc:461 > #2 0x000055555578a0b6 in linux_process_target::thread_handle (this= =3D0x5555558a64c0 , ptid=3D, handle=3D0x7fffffffc400, > handle_len=3D0x7fffffffc3f0) at /home/smarchi/src/binutils-gdb/gd= bserver/linux-low.cc:6905 > #3 0x00005555556dfcc6 in handle_qxfer_threads_worker (thread=3D0x60b= 000000510, buffer=3D0x7fffffffc8a0) at /home/smarchi/src/binutils-gdb/gdbse= rver/server.cc:1645 > #4 0x00005555556e00e6 in operator() (__closure=3D0x7fffffffc5e0, thr= ead=3D0x60b000000510) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc= :1696 > #5 0x00005555556f54be in for_each_thread >(struct {...}) (func=3D...) at /home/sma= rchi/src/binutils-gdb/gdbserver/gdbthread.h:159 > #6 0x00005555556e0242 in handle_qxfer_threads_proper (buffer=3D0x7ff= fffffc8a0) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:1694 > #7 0x00005555556e04ba in handle_qxfer_threads (annex=3D0x62900000021= 3 "", readbuf=3D0x621000019100 '\276' ..., writebuf=3D0x= 0, offset=3D0, len=3D4097) > at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:1732 > #8 0x00005555556e1989 in handle_qxfer (own_buf=3D0x629000000200 "qXf= er:threads", packet_len=3D26, new_packet_len_p=3D0x7fffffffd630) at /home/s= marchi/src/binutils-gdb/gdbserver/server.cc:2045 > #9 0x00005555556e720a in handle_query (own_buf=3D0x629000000200 "qXf= er:threads", packet_len=3D26, new_packet_len_p=3D0x7fffffffd630) at /home/s= marchi/src/binutils-gdb/gdbserver/server.cc:2685 > #10 0x00005555556f1a01 in process_serial_event () at /home/smarchi/sr= c/binutils-gdb/gdbserver/server.cc:4176 > #11 0x00005555556f4457 in handle_serial_event (err=3D0, client_data= =3D0x0) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4514 > #12 0x0000555555820f56 in handle_file_event (file_ptr=3D0x60700000025= 0, ready_mask=3D1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.= cc:573 > #13 0x0000555555821895 in gdb_wait_for_event (block=3D1) at /home/sma= rchi/src/binutils-gdb/gdbsupport/event-loop.cc:694 > #14 0x000055555581f533 in gdb_do_one_event (mstimeout=3D-1) at /home/= smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:264 > #15 0x00005555556ec9fb in start_event_loop () at /home/smarchi/src/bi= nutils-gdb/gdbserver/server.cc:3512 > #16 0x00005555556f0769 in captured_main (argc=3D4, argv=3D0x7fffffffe= 0d8) at /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3992 > #17 0x00005555556f0e3f in main (argc=3D4, argv=3D0x7fffffffe0d8) at /= home/smarchi/src/binutils-gdb/gdbserver/server.cc:4078 > > The reason is a wrong current process when find_one_thread is called. > The current process is the 2nd one, which was just attached. It does > not yet have thread_db data (proc->priv->thread_db is nullptr). As we > iterate on all threads of all process to fulfull the qxfer:threads:read > request, we get to a thread of process 1 for which we haven't read > thread_db information yet (lwp_info::thread_known is false), so we get > into find_one_thread. find_one_thread uses > `current_process=C2=A0()->priv->thread_db`, assuming the current process > matches the ptid passed as a parameter, which is wrong. A segfault > happens when trying to dereference that thread_db pointer. > > Fix this by making find_one_thread not assume what the current process / > current thread is. If it needs to call into libthread_db, which we know > will try to read memory from the current process, then temporarily set > the current process. > > In the case where the thread is already know and we return early, we > don't need to switch process. Thanks for taking the time to write a test just for this issue. Initially, I still couldn't reproduce this issue. I took the time to dig into this a little more. My default desktop install is pretty old now. And it turns out that for some reason gdbserver built in that environment can't correctly load libthread_db, though I have no idea why. The libthread_db and libpthreads are from the same glibc install, gdbserver does dlopen the library correctly, but the td_ta_new_p call fails. Looking at the error code it appears that libthread_db fails to find the version symbol (I guess it's looking for that symbol in libpthreads, maybe?) Anyway, I write the above just for a record. I retried your patch on a much more recent install, and everything works fine, I can see the failure when I back-out the GDB change, and all works fine with the fix in place. I did spot one minor style issue, detailed inline below... > > Add a test to reproduce this specific situation. > > Change-Id: I09b00883e8b73b7e5f89d0f47cb4e9c0f3d6caaa > --- > .../gdb.multi/attach-while-running.c | 26 +++++++ > .../gdb.multi/attach-while-running.exp | 69 +++++++++++++++++++ > gdbserver/thread-db.cc | 29 ++++---- > 3 files changed, 112 insertions(+), 12 deletions(-) > create mode 100644 gdb/testsuite/gdb.multi/attach-while-running.c > create mode 100644 gdb/testsuite/gdb.multi/attach-while-running.exp > > diff --git a/gdb/testsuite/gdb.multi/attach-while-running.c b/gdb/testsui= te/gdb.multi/attach-while-running.c > new file mode 100644 > index 000000000000..dd321dfe0071 > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/attach-while-running.c > @@ -0,0 +1,26 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2022 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 .= */ > + > +#include > + > +int global_var =3D 123; > + > +int > +main (void) > +{ > + sleep (30); > +} > diff --git a/gdb/testsuite/gdb.multi/attach-while-running.exp b/gdb/tests= uite/gdb.multi/attach-while-running.exp > new file mode 100644 > index 000000000000..db2877dbebe5 > --- /dev/null > +++ b/gdb/testsuite/gdb.multi/attach-while-running.exp > @@ -0,0 +1,69 @@ > +# Copyright -2022 Free Software Foundation, Inc. Extra '-' in the copyright line. Otherwise, this all looks good. Thanks, Andrew > + > +# 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 test was introduced to reproduce a specific bug in GDBserver, whe= re > +# attaching an inferior while another one was running would trigger a se= gfault > +# in GDBserver. Reproducing the bug required specific circumstances: > +# > +# - The first process must be far enough to have loaded its libc or > +# libpthread (whatever triggers the loading of libthread_db), such th= at > +# its proc->priv->thread_db is not nullptr > +# > +# - However, its lwp must still be in the `!lwp->thread_known` state, > +# meaning GDBserver hasn't asked libthread_db to compute the thread > +# handle yet. That means, GDB must not have refreshed the thread lis= t > +# yet, since that would cause the thread handles to be computed. Tha= t > +# means, no stopping on a breakpoint, since that causes a thread list > +# update. That's why the first inferior needs to be started with "ru= n > +# &". > +# > +# - Attaching the second process would segfault GDBserver. > +# > +# All of this to say, if modifying this test, please keep in mind the or= iginal > +# intent. > + > +standard_testfile > + > +if [use_gdb_stub] { > + unsupported "test requires running" > + return > +} > + > +if { [build_executable "failed to prepare" ${testfile} ${srcfile}] } { > + return > +} > + > +proc do_test {} { > + save_vars { $::GDBFLAGS } { > +=09append ::GDBFLAGS " -ex \"maint set target-non-stop on\"" > +=09clean_restart $::binfile > + } > + > + gdb_test -no-prompt-anchor "run &" > + gdb_test "add-inferior" "Added inferior 2 on connection 1 .*" > + gdb_test "inferior 2" "Switching to inferior 2 .*" > + > + set spawn_id [spawn_wait_for_attach $::binfile] > + set pid [spawn_id_get_pid $spawn_id] > + > + # This call would crash GDBserver. > + gdb_attach $pid > + > + # Read a variable from the inferior, just to make sure the attach wo= rked > + # fine. > + gdb_test "print global_var" " =3D 123" > +} > + > +do_test > diff --git a/gdbserver/thread-db.cc b/gdbserver/thread-db.cc > index 6e0e2228a5f9..bf98ca9557a5 100644 > --- a/gdbserver/thread-db.cc > +++ b/gdbserver/thread-db.cc > @@ -155,30 +155,35 @@ thread_db_state_str (td_thr_state_e state) > } > #endif > =20 > -/* Get thread info about PTID, accessing memory via the current > - thread. */ > +/* Get thread info about PTID. */ > =20 > static int > find_one_thread (ptid_t ptid) > { > - td_thrhandle_t th; > - td_thrinfo_t ti; > - td_err_e err; > - struct lwp_info *lwp; > - struct thread_db *thread_db =3D current_process ()->priv->thread_db; > - int lwpid =3D ptid.lwp (); > - > thread_info *thread =3D find_thread_ptid (ptid); > - lwp =3D get_thread_lwp (thread); > + lwp_info *lwp =3D get_thread_lwp (thread); > if (lwp->thread_known) > return 1; > =20 > - /* Get information about this thread. */ > - err =3D thread_db->td_ta_map_lwp2thr_p (thread_db->thread_agent, lwpid= , &th); > + /* Get information about this thread. libthread_db will need to read = some > + memory, which will be done on the current process, so make PTID's p= rocess > + the current one. */ > + process_info *proc =3D find_process_pid (ptid.pid ()); > + gdb_assert (proc !=3D nullptr); > + > + scoped_restore_current_thread restore_thread; > + switch_to_process (proc); > + > + thread_db *thread_db =3D proc->priv->thread_db; > + td_thrhandle_t th; > + int lwpid =3D ptid.lwp (); > + td_err_e err =3D thread_db->td_ta_map_lwp2thr_p (thread_db->thread_age= nt, lwpid, > +=09=09=09=09=09=09 &th); > if (err !=3D TD_OK) > error ("Cannot get thread handle for LWP %d: %s", > =09 lwpid, thread_db_err_str (err)); > =20 > + td_thrinfo_t ti; > err =3D thread_db->td_thr_get_info_p (&th, &ti); > if (err !=3D TD_OK) > error ("Cannot get thread info for LWP %d: %s", > --=20 > 2.38.1