public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported
@ 2022-02-09  7:58 Tankut Baris Aktemur
  2022-02-22  7:33 ` Aktemur, Tankut Baris
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Tankut Baris Aktemur @ 2022-02-09  7:58 UTC (permalink / raw)
  To: gdb-patches

When parsing the ptid out of a reply package, if the multi-process
extensions are not supported, use current_inferior's pid as the pid of
the reported thread, instead of inferior_ptid.  This is needed because
the inferior_ptid may be null_ptid although a legit context exists,
due to a prior context switch via switch_to_inferior_no_thread.

Below is a scenario that illustrates what could go wrong.  First,
setup a multi-target scenario.  This is needed, because in a
multi-target setting, the inferior_ptid is cleared out before waiting
on targets.  The second inferior below sits on top of a remote target.
Multi-process packets are disabled.

  $ # First, spawn a process with PID 26253 to attach to later.
  $ gdb-up a.out
  Reading symbols from a.out...
  (gdb) maint set target-non-stop on
  (gdb) set remote multiprocess-feature-packet off
  (gdb) start
  ...
  (gdb) add-inferior -no-connection
  [New inferior 2]
  Added inferior 2
  (gdb) inferior 2
  [Switching to inferior 2 [<null>] (<noexec>)]
  (gdb) target extended-remote | gdbserver --multi -
  Remote debugging using | gdbserver --multi -
  Remote debugging using stdio
  (gdb) attach 26253
  Attaching to Remote target
  Attached; pid = 26253
  [New Thread 26253]
  [New inferior 3]
  Reading /tmp/a.out from remote target...
  ...
  [New Thread 26253]
  ...
  Reading /usr/local/lib/debug/....debug from remote target...
  >>> GDB seems to hang here.

After attaching to a process and reading some library files, GDB
seems to hang.  One interesting thing to note is that

  [New Thread 26253]

appears twice.  We also see

  [New inferior 3]

Running the same scenario with "debug infrun on" reveals more details.

  ...
  (gdb) attach 26253
  [infrun] scoped_disable_commit_resumed: reason=attaching
  Attaching to Remote target
  Attached; pid = 26253
  [New Thread 26253]
  [infrun] infrun_async: enable=1
  [infrun] attach_command: immediately after attach:
  [infrun] attach_command:   thread 26253.26253.0, executing = 1, resumed = 0, state = RUNNING
  [infrun] clear_proceed_status_thread: 26253.26253.0
  [infrun] reset: reason=attaching
  [infrun] maybe_set_commit_resumed_all_targets: not requesting commit-resumed for target native, no resumed threads
  [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target extended-remote
  [infrun] fetch_inferior_event: enter
    [infrun] scoped_disable_commit_resumed: reason=handling event
    [infrun] do_target_wait: Found 2 inferiors, starting at #1
    [infrun] random_pending_event_thread: None found.
    [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
    [infrun] print_target_wait_results:   26253.26253.0 [Thread 26253],
    [infrun] print_target_wait_results:   status->kind = STOPPED, sig = GDB_SIGNAL_0
    [infrun] handle_inferior_event: status->kind = STOPPED, sig = GDB_SIGNAL_0
    [infrun] start_step_over: enter
      [infrun] start_step_over: stealing global queue of threads to step, length = 0
      [infrun] operator(): step-over queue now empty
    [infrun] start_step_over: exit
    [infrun] context_switch: Switching context from 0.0.0 to 26253.26253.0
    [infrun] handle_signal_stop: stop_pc=0x7f849d8cf151
    [infrun] stop_waiting: stop_waiting
    [infrun] stop_all_threads: starting
    [infrun] stop_all_threads: pass=0, iterations=0
  [New inferior 3]
  Reading /tmp/a.out from remote target...
  warning: File transfers from remote targets can be slow. Use "set sysroot" to access files locally instead.
  Reading /tmp/a.out from remote target...
  Reading symbols from target:/tmp/a.out...
  [New Thread 26253]
    [infrun] stop_all_threads:   4723.4723.0 not executing
    [infrun] stop_all_threads:   26253.26253.0 not executing
    [infrun] stop_all_threads:   42000.26253.0 executing, need stop
    [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
    [infrun] print_target_wait_results:   -1.0.0 [Thread 0],
    [infrun] print_target_wait_results:   status->kind = IGNORE
    [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
    [infrun] print_target_wait_results:   -1.0.0 [Thread 0],
    [infrun] print_target_wait_results:   status->kind = IGNORE

GDB tried to stop Thread 42000.26253.0, which does not exist, and we
are waiting for a stop event that will never happen.  The PID in
'42000.26253.0', namely 42000, is the PID of magic_null_ptid.
It comes from gdb/remote.c:read_ptid:

  /* Since the stub is not sending a process id, then default to
     what's in inferior_ptid, unless it's null at this point.  If so,
     then since there's no way to know the pid of the reported
     threads, use the magic number.  */
  if (inferior_ptid == null_ptid)
    pid = magic_null_ptid.pid ();
  else
    pid = inferior_ptid.pid ();

  if (obuf)
    *obuf = pp;
  return ptid_t (pid, tid);

Because multi-process was turned off, GDB did not parse an explicitly
specified PID.  Furthermore, inferior_ptid == null_ptid, and
eventually GDB picked the PID from magic_null_ptid.

If target-non-stop is not turned on at the beginning, the same bug
reveals itself as a duplicated thread as shown below.

  # Same setup as above, without 'maint set target-non-stop on'.
  ...
  (gdb) attach 26253
  Attaching to Remote target
  Attached; pid = 26253
  [New inferior 3]
  ...
  [New Thread 26253]
  ...
  (gdb) info threads
    Id   Target Id             Frame
    1.1  process 13517 "a.out" main () at test.c:3
  * 2.1  Thread 26253 "a.out"  0x00007f12750c5151 in read () from target:/lib/x86_64-linux-gnu/libc.so.6
    3.1  Thread 26253 "a.out"  Remote 'g' packet reply is too long (expected 560 bytes, got 2496 bytes): 00feffffffffffff000a3a75127f000051510c75127f0000000400000000000060d24ef6af5500000000000000000000680d000000000000b85b31e3fc7f0000c0283a75127f000000e55b75127f000010d04ef6af550000460200000000000060c73975127f0000a0d23975127f0000000a3a75127f0000000000000000000051510c75127f000046020000330000002b0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007f03000000000000ffff0000000000000000000000000000000000000000000080143a75127f000080143a75127f000040143a75127f000040143a75127f00007d0000007e0000007f00000080000000300c3a75127f0000300c3a75127f00000e000000000000000e0000000000000000000000000000000000000000000000ffffffffffffffffffffffffffffffff0400000004000000040000000400000020143a75127f000020143a75127f000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000801f0000000000000000000000e55b75127f0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
  (gdb)

Fix the problem by preferring current_inferior()'s pid instead of
magic_null_ptid.

Regression-tested on X86-64 Linux.

Co-authored-by: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
---
 gdb/remote.c                                  |  9 +-
 .../gdb.multi/attach-no-multi-process.c       | 34 +++++++
 .../gdb.multi/attach-no-multi-process.exp     | 90 +++++++++++++++++++
 3 files changed, 129 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.multi/attach-no-multi-process.c
 create mode 100644 gdb/testsuite/gdb.multi/attach-no-multi-process.exp

diff --git a/gdb/remote.c b/gdb/remote.c
index 49eeb63445d..281260af971 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3145,14 +3145,15 @@ read_ptid (const char *buf, const char **obuf)
       return null_ptid;
     }
 
-  /* Since the stub is not sending a process id, then default to
-     what's in inferior_ptid, unless it's null at this point.  If so,
+  /* Since the stub is not sending a process id, default to
+     what's current_inferior, unless it's null at this point.  If so,
      then since there's no way to know the pid of the reported
      threads, use the magic number.  */
-  if (inferior_ptid == null_ptid)
+  inferior *inf = current_inferior ();
+  if (inf == nullptr || inf->pid == 0)
     pid = magic_null_ptid.pid ();
   else
-    pid = inferior_ptid.pid ();
+    pid = inf->pid;
 
   if (obuf)
     *obuf = pp;
diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.c b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
new file mode 100644
index 00000000000..df09dd69ccc
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
@@ -0,0 +1,34 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020-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 <http://www.gnu.org/licenses/>.  */
+
+/* This program is intended to be started outside of gdb, and then
+   attached to by GDB.  */
+
+#include <unistd.h>
+
+int
+main ()
+{
+  /* Don't run forever in case GDB crashes and DejaGNU fails to kill
+     this program.  */
+  alarm (10);
+
+  while (1)
+    ;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.exp b/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
new file mode 100644
index 00000000000..0ba99351ec5
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
@@ -0,0 +1,90 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2020-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 <http://www.gnu.org/licenses/>.
+
+# Test attaching to a process, as a second inferior, through a
+# gdbserver that does not support multi-process extensions.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+# The plain remote target can't do multiple inferiors.
+if {[target_info gdb_protocol] != ""} {
+    return
+}
+
+if {![can_spawn_for_attach]} {
+    return
+}
+
+if {[build_executable "build" $testfile $srcfile {debug}] == -1} {
+    return -1
+}
+
+proc test {target_non_stop} {
+    global binfile
+
+    save_vars { ::GDBFLAGS } {
+	# Make GDB read files from the local file system, not through the
+	# remote targets, to speed things up.
+	set ::GDBFLAGS "${::GDBFLAGS} -ex \"set sysroot\""
+	clean_restart ${binfile}
+    }
+
+    gdb_test_no_output "set remote multiprocess-feature-packet off"
+    gdb_test_no_output "maint set target-non-stop ${target_non_stop}"
+
+    # Start the first inferior.
+    if {![runto_main]} {
+	return
+    }
+
+    # The second inferior is an extended remote.
+    gdb_test "add-inferior -no-connection" "Added inferior 2.*" \
+	"add the second inferior"
+    gdb_test "inferior 2" ".*Switching to inferior 2.*" \
+	"switch to inferior 2"
+    set res [gdbserver_start "--multi" ""]
+    set gdbserver_gdbport [lindex $res 1]
+    gdb_target_cmd "extended-remote" $gdbserver_gdbport
+
+    # Start a program, then attach to it.
+    set spawn_id_list [spawn_wait_for_attach [list $binfile]]
+    set test_spawn_id [lindex $spawn_id_list 0]
+    set testpid [spawn_id_get_pid $test_spawn_id]
+    gdb_test "attach $testpid" \
+	"Attaching to Remote target.*" \
+	"attach to the program via remote"
+
+    # Check that we have two threads.  Bad GDB duplicated the
+    # thread coming from the remote when target-non-stop is off;
+    # or hanged during attach when target-non-stop is on.
+    gdb_test "info threads" \
+	[multi_line \
+	     "  Id\[^\r\n\]+" \
+	     "  1\.1\[^\r\n\]+" \
+	     ". 2\.1\[^\r\n\]+"
+	]
+
+    # Clean the spawned process and gdbserver.
+    gdbserver_exit 0
+    kill_wait_spawned_process $test_spawn_id
+}
+
+foreach_with_prefix target_non_stop {off on} {
+    test $target_non_stop
+}
-- 
2.33.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported
  2022-02-09  7:58 [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported Tankut Baris Aktemur
@ 2022-02-22  7:33 ` Aktemur, Tankut Baris
  2022-03-07  7:59 ` Aktemur, Tankut Baris
  2022-03-07 16:03 ` Pedro Alves
  2 siblings, 0 replies; 5+ messages in thread
From: Aktemur, Tankut Baris @ 2022-02-22  7:33 UTC (permalink / raw)
  To: gdb-patches

Kindly pinging. 

Thanks
-Baris

On Wednesday, February 9, 2022 8:58 AM, Aktemur, Tankut Baris wrote:
> When parsing the ptid out of a reply package, if the multi-process
> extensions are not supported, use current_inferior's pid as the pid of
> the reported thread, instead of inferior_ptid.  This is needed because
> the inferior_ptid may be null_ptid although a legit context exists,
> due to a prior context switch via switch_to_inferior_no_thread.
> 
> Below is a scenario that illustrates what could go wrong.  First,
> setup a multi-target scenario.  This is needed, because in a
> multi-target setting, the inferior_ptid is cleared out before waiting
> on targets.  The second inferior below sits on top of a remote target.
> Multi-process packets are disabled.
> 
>   $ # First, spawn a process with PID 26253 to attach to later.
>   $ gdb-up a.out
>   Reading symbols from a.out...
>   (gdb) maint set target-non-stop on
>   (gdb) set remote multiprocess-feature-packet off
>   (gdb) start
>   ...
>   (gdb) add-inferior -no-connection
>   [New inferior 2]
>   Added inferior 2
>   (gdb) inferior 2
>   [Switching to inferior 2 [<null>] (<noexec>)]
>   (gdb) target extended-remote | gdbserver --multi -
>   Remote debugging using | gdbserver --multi -
>   Remote debugging using stdio
>   (gdb) attach 26253
>   Attaching to Remote target
>   Attached; pid = 26253
>   [New Thread 26253]
>   [New inferior 3]
>   Reading /tmp/a.out from remote target...
>   ...
>   [New Thread 26253]
>   ...
>   Reading /usr/local/lib/debug/....debug from remote target...
>   >>> GDB seems to hang here.
> 
> After attaching to a process and reading some library files, GDB
> seems to hang.  One interesting thing to note is that
> 
>   [New Thread 26253]
> 
> appears twice.  We also see
> 
>   [New inferior 3]
> 
> Running the same scenario with "debug infrun on" reveals more details.
> 
>   ...
>   (gdb) attach 26253
>   [infrun] scoped_disable_commit_resumed: reason=attaching
>   Attaching to Remote target
>   Attached; pid = 26253
>   [New Thread 26253]
>   [infrun] infrun_async: enable=1
>   [infrun] attach_command: immediately after attach:
>   [infrun] attach_command:   thread 26253.26253.0, executing = 1, resumed = 0, state =
> RUNNING
>   [infrun] clear_proceed_status_thread: 26253.26253.0
>   [infrun] reset: reason=attaching
>   [infrun] maybe_set_commit_resumed_all_targets: not requesting commit-resumed for target
> native, no resumed threads
>   [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target
> extended-remote
>   [infrun] fetch_inferior_event: enter
>     [infrun] scoped_disable_commit_resumed: reason=handling event
>     [infrun] do_target_wait: Found 2 inferiors, starting at #1
>     [infrun] random_pending_event_thread: None found.
>     [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
>     [infrun] print_target_wait_results:   26253.26253.0 [Thread 26253],
>     [infrun] print_target_wait_results:   status->kind = STOPPED, sig = GDB_SIGNAL_0
>     [infrun] handle_inferior_event: status->kind = STOPPED, sig = GDB_SIGNAL_0
>     [infrun] start_step_over: enter
>       [infrun] start_step_over: stealing global queue of threads to step, length = 0
>       [infrun] operator(): step-over queue now empty
>     [infrun] start_step_over: exit
>     [infrun] context_switch: Switching context from 0.0.0 to 26253.26253.0
>     [infrun] handle_signal_stop: stop_pc=0x7f849d8cf151
>     [infrun] stop_waiting: stop_waiting
>     [infrun] stop_all_threads: starting
>     [infrun] stop_all_threads: pass=0, iterations=0
>   [New inferior 3]
>   Reading /tmp/a.out from remote target...
>   warning: File transfers from remote targets can be slow. Use "set sysroot" to access files
> locally instead.
>   Reading /tmp/a.out from remote target...
>   Reading symbols from target:/tmp/a.out...
>   [New Thread 26253]
>     [infrun] stop_all_threads:   4723.4723.0 not executing
>     [infrun] stop_all_threads:   26253.26253.0 not executing
>     [infrun] stop_all_threads:   42000.26253.0 executing, need stop
>     [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
>     [infrun] print_target_wait_results:   -1.0.0 [Thread 0],
>     [infrun] print_target_wait_results:   status->kind = IGNORE
>     [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
>     [infrun] print_target_wait_results:   -1.0.0 [Thread 0],
>     [infrun] print_target_wait_results:   status->kind = IGNORE
> 
> GDB tried to stop Thread 42000.26253.0, which does not exist, and we
> are waiting for a stop event that will never happen.  The PID in
> '42000.26253.0', namely 42000, is the PID of magic_null_ptid.
> It comes from gdb/remote.c:read_ptid:
> 
>   /* Since the stub is not sending a process id, then default to
>      what's in inferior_ptid, unless it's null at this point.  If so,
>      then since there's no way to know the pid of the reported
>      threads, use the magic number.  */
>   if (inferior_ptid == null_ptid)
>     pid = magic_null_ptid.pid ();
>   else
>     pid = inferior_ptid.pid ();
> 
>   if (obuf)
>     *obuf = pp;
>   return ptid_t (pid, tid);
> 
> Because multi-process was turned off, GDB did not parse an explicitly
> specified PID.  Furthermore, inferior_ptid == null_ptid, and
> eventually GDB picked the PID from magic_null_ptid.
> 
> If target-non-stop is not turned on at the beginning, the same bug
> reveals itself as a duplicated thread as shown below.
> 
>   # Same setup as above, without 'maint set target-non-stop on'.
>   ...
>   (gdb) attach 26253
>   Attaching to Remote target
>   Attached; pid = 26253
>   [New inferior 3]
>   ...
>   [New Thread 26253]
>   ...
>   (gdb) info threads
>     Id   Target Id             Frame
>     1.1  process 13517 "a.out" main () at test.c:3
>   * 2.1  Thread 26253 "a.out"  0x00007f12750c5151 in read () from target:/lib/x86_64-linux-
> gnu/libc.so.6
>     3.1  Thread 26253 "a.out"  Remote 'g' packet reply is too long (expected 560 bytes, got
> 2496 bytes):
> 00feffffffffffff000a3a75127f000051510c75127f0000000400000000000060d24ef6af550000000000000000
> 0000680d000000000000b85b31e3fc7f0000c0283a75127f000000e55b75127f000010d04ef6af55000046020000
> 0000000060c73975127f0000a0d23975127f0000000a3a75127f0000000000000000000051510c75127f00004602
> 0000330000002b000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000007f03000000000000ffff00000000000000000000000000000000000000000000
> 80143a75127f000080143a75127f000040143a75127f000040143a75127f00007d0000007e0000007f0000008000
> 0000300c3a75127f0000300c3a75127f00000e000000000000000e00000000000000000000000000000000000000
> 00000000ffffffffffffffffffffffffffffffff0400000004000000040000000400000020143a75127f00002014
> 3a75127f000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 0000000000000000000000000000000000000000000000000000801f0000000000000000000000e55b75127f0000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 000000000000000000000000
>   (gdb)
> 
> Fix the problem by preferring current_inferior()'s pid instead of
> magic_null_ptid.
> 
> Regression-tested on X86-64 Linux.
> 
> Co-authored-by: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
> ---
>  gdb/remote.c                                  |  9 +-
>  .../gdb.multi/attach-no-multi-process.c       | 34 +++++++
>  .../gdb.multi/attach-no-multi-process.exp     | 90 +++++++++++++++++++
>  3 files changed, 129 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.multi/attach-no-multi-process.c
>  create mode 100644 gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 49eeb63445d..281260af971 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3145,14 +3145,15 @@ read_ptid (const char *buf, const char **obuf)
>        return null_ptid;
>      }
> 
> -  /* Since the stub is not sending a process id, then default to
> -     what's in inferior_ptid, unless it's null at this point.  If so,
> +  /* Since the stub is not sending a process id, default to
> +     what's current_inferior, unless it's null at this point.  If so,
>       then since there's no way to know the pid of the reported
>       threads, use the magic number.  */
> -  if (inferior_ptid == null_ptid)
> +  inferior *inf = current_inferior ();
> +  if (inf == nullptr || inf->pid == 0)
>      pid = magic_null_ptid.pid ();
>    else
> -    pid = inferior_ptid.pid ();
> +    pid = inf->pid;
> 
>    if (obuf)
>      *obuf = pp;
> diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> new file mode 100644
> index 00000000000..df09dd69ccc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> @@ -0,0 +1,34 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020-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 <http://www.gnu.org/licenses/>.  */
> +
> +/* This program is intended to be started outside of gdb, and then
> +   attached to by GDB.  */
> +
> +#include <unistd.h>
> +
> +int
> +main ()
> +{
> +  /* Don't run forever in case GDB crashes and DejaGNU fails to kill
> +     this program.  */
> +  alarm (10);
> +
> +  while (1)
> +    ;
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> b/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> new file mode 100644
> index 00000000000..0ba99351ec5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> @@ -0,0 +1,90 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2020-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 <http://www.gnu.org/licenses/>.
> +
> +# Test attaching to a process, as a second inferior, through a
> +# gdbserver that does not support multi-process extensions.
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +# The plain remote target can't do multiple inferiors.
> +if {[target_info gdb_protocol] != ""} {
> +    return
> +}
> +
> +if {![can_spawn_for_attach]} {
> +    return
> +}
> +
> +if {[build_executable "build" $testfile $srcfile {debug}] == -1} {
> +    return -1
> +}
> +
> +proc test {target_non_stop} {
> +    global binfile
> +
> +    save_vars { ::GDBFLAGS } {
> +	# Make GDB read files from the local file system, not through the
> +	# remote targets, to speed things up.
> +	set ::GDBFLAGS "${::GDBFLAGS} -ex \"set sysroot\""
> +	clean_restart ${binfile}
> +    }
> +
> +    gdb_test_no_output "set remote multiprocess-feature-packet off"
> +    gdb_test_no_output "maint set target-non-stop ${target_non_stop}"
> +
> +    # Start the first inferior.
> +    if {![runto_main]} {
> +	return
> +    }
> +
> +    # The second inferior is an extended remote.
> +    gdb_test "add-inferior -no-connection" "Added inferior 2.*" \
> +	"add the second inferior"
> +    gdb_test "inferior 2" ".*Switching to inferior 2.*" \
> +	"switch to inferior 2"
> +    set res [gdbserver_start "--multi" ""]
> +    set gdbserver_gdbport [lindex $res 1]
> +    gdb_target_cmd "extended-remote" $gdbserver_gdbport
> +
> +    # Start a program, then attach to it.
> +    set spawn_id_list [spawn_wait_for_attach [list $binfile]]
> +    set test_spawn_id [lindex $spawn_id_list 0]
> +    set testpid [spawn_id_get_pid $test_spawn_id]
> +    gdb_test "attach $testpid" \
> +	"Attaching to Remote target.*" \
> +	"attach to the program via remote"
> +
> +    # Check that we have two threads.  Bad GDB duplicated the
> +    # thread coming from the remote when target-non-stop is off;
> +    # or hanged during attach when target-non-stop is on.
> +    gdb_test "info threads" \
> +	[multi_line \
> +	     "  Id\[^\r\n\]+" \
> +	     "  1\.1\[^\r\n\]+" \
> +	     ". 2\.1\[^\r\n\]+"
> +	]
> +
> +    # Clean the spawned process and gdbserver.
> +    gdbserver_exit 0
> +    kill_wait_spawned_process $test_spawn_id
> +}
> +
> +foreach_with_prefix target_non_stop {off on} {
> +    test $target_non_stop
> +}
> --
> 2.33.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported
  2022-02-09  7:58 [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported Tankut Baris Aktemur
  2022-02-22  7:33 ` Aktemur, Tankut Baris
@ 2022-03-07  7:59 ` Aktemur, Tankut Baris
  2022-03-07 16:03 ` Pedro Alves
  2 siblings, 0 replies; 5+ messages in thread
From: Aktemur, Tankut Baris @ 2022-03-07  7:59 UTC (permalink / raw)
  To: gdb-patches

Kindly pinging. 

Thanks
-Baris

On Wednesday, February 9, 2022 8:58 AM, Aktemur, Tankut Baris wrote:
> When parsing the ptid out of a reply package, if the multi-process
> extensions are not supported, use current_inferior's pid as the pid of
> the reported thread, instead of inferior_ptid.  This is needed because
> the inferior_ptid may be null_ptid although a legit context exists,
> due to a prior context switch via switch_to_inferior_no_thread.
> 
> Below is a scenario that illustrates what could go wrong.  First,
> setup a multi-target scenario.  This is needed, because in a
> multi-target setting, the inferior_ptid is cleared out before waiting
> on targets.  The second inferior below sits on top of a remote target.
> Multi-process packets are disabled.
> 
>   $ # First, spawn a process with PID 26253 to attach to later.
>   $ gdb-up a.out
>   Reading symbols from a.out...
>   (gdb) maint set target-non-stop on
>   (gdb) set remote multiprocess-feature-packet off
>   (gdb) start
>   ...
>   (gdb) add-inferior -no-connection
>   [New inferior 2]
>   Added inferior 2
>   (gdb) inferior 2
>   [Switching to inferior 2 [<null>] (<noexec>)]
>   (gdb) target extended-remote | gdbserver --multi -
>   Remote debugging using | gdbserver --multi -
>   Remote debugging using stdio
>   (gdb) attach 26253
>   Attaching to Remote target
>   Attached; pid = 26253
>   [New Thread 26253]
>   [New inferior 3]
>   Reading /tmp/a.out from remote target...
>   ...
>   [New Thread 26253]
>   ...
>   Reading /usr/local/lib/debug/....debug from remote target...
>   >>> GDB seems to hang here.
> 
> After attaching to a process and reading some library files, GDB
> seems to hang.  One interesting thing to note is that
> 
>   [New Thread 26253]
> 
> appears twice.  We also see
> 
>   [New inferior 3]
> 
> Running the same scenario with "debug infrun on" reveals more details.
> 
>   ...
>   (gdb) attach 26253
>   [infrun] scoped_disable_commit_resumed: reason=attaching
>   Attaching to Remote target
>   Attached; pid = 26253
>   [New Thread 26253]
>   [infrun] infrun_async: enable=1
>   [infrun] attach_command: immediately after attach:
>   [infrun] attach_command:   thread 26253.26253.0, executing = 1, resumed = 0, state =
> RUNNING
>   [infrun] clear_proceed_status_thread: 26253.26253.0
>   [infrun] reset: reason=attaching
>   [infrun] maybe_set_commit_resumed_all_targets: not requesting commit-resumed for target
> native, no resumed threads
>   [infrun] maybe_set_commit_resumed_all_targets: enabling commit-resumed for target
> extended-remote
>   [infrun] fetch_inferior_event: enter
>     [infrun] scoped_disable_commit_resumed: reason=handling event
>     [infrun] do_target_wait: Found 2 inferiors, starting at #1
>     [infrun] random_pending_event_thread: None found.
>     [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
>     [infrun] print_target_wait_results:   26253.26253.0 [Thread 26253],
>     [infrun] print_target_wait_results:   status->kind = STOPPED, sig = GDB_SIGNAL_0
>     [infrun] handle_inferior_event: status->kind = STOPPED, sig = GDB_SIGNAL_0
>     [infrun] start_step_over: enter
>       [infrun] start_step_over: stealing global queue of threads to step, length = 0
>       [infrun] operator(): step-over queue now empty
>     [infrun] start_step_over: exit
>     [infrun] context_switch: Switching context from 0.0.0 to 26253.26253.0
>     [infrun] handle_signal_stop: stop_pc=0x7f849d8cf151
>     [infrun] stop_waiting: stop_waiting
>     [infrun] stop_all_threads: starting
>     [infrun] stop_all_threads: pass=0, iterations=0
>   [New inferior 3]
>   Reading /tmp/a.out from remote target...
>   warning: File transfers from remote targets can be slow. Use "set sysroot" to access files
> locally instead.
>   Reading /tmp/a.out from remote target...
>   Reading symbols from target:/tmp/a.out...
>   [New Thread 26253]
>     [infrun] stop_all_threads:   4723.4723.0 not executing
>     [infrun] stop_all_threads:   26253.26253.0 not executing
>     [infrun] stop_all_threads:   42000.26253.0 executing, need stop
>     [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
>     [infrun] print_target_wait_results:   -1.0.0 [Thread 0],
>     [infrun] print_target_wait_results:   status->kind = IGNORE
>     [infrun] print_target_wait_results: target_wait (-1.0.0 [Thread 0], status) =
>     [infrun] print_target_wait_results:   -1.0.0 [Thread 0],
>     [infrun] print_target_wait_results:   status->kind = IGNORE
> 
> GDB tried to stop Thread 42000.26253.0, which does not exist, and we
> are waiting for a stop event that will never happen.  The PID in
> '42000.26253.0', namely 42000, is the PID of magic_null_ptid.
> It comes from gdb/remote.c:read_ptid:
> 
>   /* Since the stub is not sending a process id, then default to
>      what's in inferior_ptid, unless it's null at this point.  If so,
>      then since there's no way to know the pid of the reported
>      threads, use the magic number.  */
>   if (inferior_ptid == null_ptid)
>     pid = magic_null_ptid.pid ();
>   else
>     pid = inferior_ptid.pid ();
> 
>   if (obuf)
>     *obuf = pp;
>   return ptid_t (pid, tid);
> 
> Because multi-process was turned off, GDB did not parse an explicitly
> specified PID.  Furthermore, inferior_ptid == null_ptid, and
> eventually GDB picked the PID from magic_null_ptid.
> 
> If target-non-stop is not turned on at the beginning, the same bug
> reveals itself as a duplicated thread as shown below.
> 
>   # Same setup as above, without 'maint set target-non-stop on'.
>   ...
>   (gdb) attach 26253
>   Attaching to Remote target
>   Attached; pid = 26253
>   [New inferior 3]
>   ...
>   [New Thread 26253]
>   ...
>   (gdb) info threads
>     Id   Target Id             Frame
>     1.1  process 13517 "a.out" main () at test.c:3
>   * 2.1  Thread 26253 "a.out"  0x00007f12750c5151 in read () from target:/lib/x86_64-linux-
> gnu/libc.so.6
>     3.1  Thread 26253 "a.out"  Remote 'g' packet reply is too long (expected 560 bytes, got
> 2496 bytes):
> 00feffffffffffff000a3a75127f000051510c75127f0000000400000000000060d24ef6af550000000000000000
> 0000680d000000000000b85b31e3fc7f0000c0283a75127f000000e55b75127f000010d04ef6af55000046020000
> 0000000060c73975127f0000a0d23975127f0000000a3a75127f0000000000000000000051510c75127f00004602
> 0000330000002b000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000007f03000000000000ffff00000000000000000000000000000000000000000000
> 80143a75127f000080143a75127f000040143a75127f000040143a75127f00007d0000007e0000007f0000008000
> 0000300c3a75127f0000300c3a75127f00000e000000000000000e00000000000000000000000000000000000000
> 00000000ffffffffffffffffffffffffffffffff0400000004000000040000000400000020143a75127f00002014
> 3a75127f000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 0000000000000000000000000000000000000000000000000000801f0000000000000000000000e55b75127f0000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
> 000000000000000000000000
>   (gdb)
> 
> Fix the problem by preferring current_inferior()'s pid instead of
> magic_null_ptid.
> 
> Regression-tested on X86-64 Linux.
> 
> Co-authored-by: Aleksandar Paunovic <aleksandar.paunovic@intel.com>
> ---
>  gdb/remote.c                                  |  9 +-
>  .../gdb.multi/attach-no-multi-process.c       | 34 +++++++
>  .../gdb.multi/attach-no-multi-process.exp     | 90 +++++++++++++++++++
>  3 files changed, 129 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.multi/attach-no-multi-process.c
>  create mode 100644 gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 49eeb63445d..281260af971 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3145,14 +3145,15 @@ read_ptid (const char *buf, const char **obuf)
>        return null_ptid;
>      }
> 
> -  /* Since the stub is not sending a process id, then default to
> -     what's in inferior_ptid, unless it's null at this point.  If so,
> +  /* Since the stub is not sending a process id, default to
> +     what's current_inferior, unless it's null at this point.  If so,
>       then since there's no way to know the pid of the reported
>       threads, use the magic number.  */
> -  if (inferior_ptid == null_ptid)
> +  inferior *inf = current_inferior ();
> +  if (inf == nullptr || inf->pid == 0)
>      pid = magic_null_ptid.pid ();
>    else
> -    pid = inferior_ptid.pid ();
> +    pid = inf->pid;
> 
>    if (obuf)
>      *obuf = pp;
> diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> new file mode 100644
> index 00000000000..df09dd69ccc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> @@ -0,0 +1,34 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020-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 <http://www.gnu.org/licenses/>.  */
> +
> +/* This program is intended to be started outside of gdb, and then
> +   attached to by GDB.  */
> +
> +#include <unistd.h>
> +
> +int
> +main ()
> +{
> +  /* Don't run forever in case GDB crashes and DejaGNU fails to kill
> +     this program.  */
> +  alarm (10);
> +
> +  while (1)
> +    ;
> +
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> b/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> new file mode 100644
> index 00000000000..0ba99351ec5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.exp
> @@ -0,0 +1,90 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2020-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 <http://www.gnu.org/licenses/>.
> +
> +# Test attaching to a process, as a second inferior, through a
> +# gdbserver that does not support multi-process extensions.
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +# The plain remote target can't do multiple inferiors.
> +if {[target_info gdb_protocol] != ""} {
> +    return
> +}
> +
> +if {![can_spawn_for_attach]} {
> +    return
> +}
> +
> +if {[build_executable "build" $testfile $srcfile {debug}] == -1} {
> +    return -1
> +}
> +
> +proc test {target_non_stop} {
> +    global binfile
> +
> +    save_vars { ::GDBFLAGS } {
> +	# Make GDB read files from the local file system, not through the
> +	# remote targets, to speed things up.
> +	set ::GDBFLAGS "${::GDBFLAGS} -ex \"set sysroot\""
> +	clean_restart ${binfile}
> +    }
> +
> +    gdb_test_no_output "set remote multiprocess-feature-packet off"
> +    gdb_test_no_output "maint set target-non-stop ${target_non_stop}"
> +
> +    # Start the first inferior.
> +    if {![runto_main]} {
> +	return
> +    }
> +
> +    # The second inferior is an extended remote.
> +    gdb_test "add-inferior -no-connection" "Added inferior 2.*" \
> +	"add the second inferior"
> +    gdb_test "inferior 2" ".*Switching to inferior 2.*" \
> +	"switch to inferior 2"
> +    set res [gdbserver_start "--multi" ""]
> +    set gdbserver_gdbport [lindex $res 1]
> +    gdb_target_cmd "extended-remote" $gdbserver_gdbport
> +
> +    # Start a program, then attach to it.
> +    set spawn_id_list [spawn_wait_for_attach [list $binfile]]
> +    set test_spawn_id [lindex $spawn_id_list 0]
> +    set testpid [spawn_id_get_pid $test_spawn_id]
> +    gdb_test "attach $testpid" \
> +	"Attaching to Remote target.*" \
> +	"attach to the program via remote"
> +
> +    # Check that we have two threads.  Bad GDB duplicated the
> +    # thread coming from the remote when target-non-stop is off;
> +    # or hanged during attach when target-non-stop is on.
> +    gdb_test "info threads" \
> +	[multi_line \
> +	     "  Id\[^\r\n\]+" \
> +	     "  1\.1\[^\r\n\]+" \
> +	     ". 2\.1\[^\r\n\]+"
> +	]
> +
> +    # Clean the spawned process and gdbserver.
> +    gdbserver_exit 0
> +    kill_wait_spawned_process $test_spawn_id
> +}
> +
> +foreach_with_prefix target_non_stop {off on} {
> +    test $target_non_stop
> +}
> --
> 2.33.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported
  2022-02-09  7:58 [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported Tankut Baris Aktemur
  2022-02-22  7:33 ` Aktemur, Tankut Baris
  2022-03-07  7:59 ` Aktemur, Tankut Baris
@ 2022-03-07 16:03 ` Pedro Alves
  2022-03-29  8:25   ` Aktemur, Tankut Baris
  2 siblings, 1 reply; 5+ messages in thread
From: Pedro Alves @ 2022-03-07 16:03 UTC (permalink / raw)
  To: Tankut Baris Aktemur, gdb-patches

On 2022-02-09 07:58, Tankut Baris Aktemur via Gdb-patches wrote:

> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3145,14 +3145,15 @@ read_ptid (const char *buf, const char **obuf)
>        return null_ptid;
>      }
>  
> -  /* Since the stub is not sending a process id, then default to
> -     what's in inferior_ptid, unless it's null at this point.  If so,
> +  /* Since the stub is not sending a process id, default to
> +     what's current_inferior, unless it's null at this point.  If so,
>       then since there's no way to know the pid of the reported
>       threads, use the magic number.  */
> -  if (inferior_ptid == null_ptid)
> +  inferior *inf = current_inferior ();
> +  if (inf == nullptr || inf->pid == 0)

How can current_inferior() ever by nullptr here?  That should not be possible, unless
you're in some very early _initialize_foo routine, before the initial inferior was
created.  There's is always an inferior selected, there's no way to select "no inferior".

>      pid = magic_null_ptid.pid ();
>    else
> -    pid = inferior_ptid.pid ();
> +    pid = inf->pid;
>  
>    if (obuf)
>      *obuf = pp;
> diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.c b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> new file mode 100644
> index 00000000000..df09dd69ccc
> --- /dev/null
> +++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> @@ -0,0 +1,34 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020-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 <http://www.gnu.org/licenses/>.  */
> +
> +/* This program is intended to be started outside of gdb, and then
> +   attached to by GDB.  */

gdb vs GDB.  Use uppercase in both cases.

> +
> +#include <unistd.h>
> +
> +int
> +main ()
> +{
> +  /* Don't run forever in case GDB crashes and DejaGNU fails to kill
> +     this program.  */
> +  alarm (10);
> +
> +  while (1)
> +    ;

Please add some "usleep (1);" or some such in the loop to avoid hogging
the CPU.

> +
> +# The plain remote target can't do multiple inferiors.

Extended-remote can.  And, you start a separate gdbserver anyhow.  What happens if
you remove this check?  You'd need to pass
"set remote multiprocess-feature-packet off" and "maint set target-non-stop"
via GDBFLAGS instead of as separate commands after starting gdb, but is there
anything else?


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported
  2022-03-07 16:03 ` Pedro Alves
@ 2022-03-29  8:25   ` Aktemur, Tankut Baris
  0 siblings, 0 replies; 5+ messages in thread
From: Aktemur, Tankut Baris @ 2022-03-29  8:25 UTC (permalink / raw)
  To: Pedro Alves, gdb-patches

On Monday, March 7, 2022 5:04 PM, Pedro Alves wrote:
> On 2022-02-09 07:58, Tankut Baris Aktemur via Gdb-patches wrote:
> 
> > --- a/gdb/remote.c
> > +++ b/gdb/remote.c
> > @@ -3145,14 +3145,15 @@ read_ptid (const char *buf, const char **obuf)
> >        return null_ptid;
> >      }
> >
> > -  /* Since the stub is not sending a process id, then default to
> > -     what's in inferior_ptid, unless it's null at this point.  If so,
> > +  /* Since the stub is not sending a process id, default to
> > +     what's current_inferior, unless it's null at this point.  If so,
> >       then since there's no way to know the pid of the reported
> >       threads, use the magic number.  */
> > -  if (inferior_ptid == null_ptid)
> > +  inferior *inf = current_inferior ();
> > +  if (inf == nullptr || inf->pid == 0)
> 
> How can current_inferior() ever by nullptr here?  That should not be possible, unless
> you're in some very early _initialize_foo routine, before the initial inferior was
> created.  There's is always an inferior selected, there's no way to select "no inferior".

You're right.  I remove the null check in v2.
 
> >      pid = magic_null_ptid.pid ();
> >    else
> > -    pid = inferior_ptid.pid ();
> > +    pid = inf->pid;
> >
> >    if (obuf)
> >      *obuf = pp;
> > diff --git a/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> > new file mode 100644
> > index 00000000000..df09dd69ccc
> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.multi/attach-no-multi-process.c
> > @@ -0,0 +1,34 @@
> > +/* This testcase is part of GDB, the GNU debugger.
> > +
> > +   Copyright 2020-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 <http://www.gnu.org/licenses/>.  */
> > +
> > +/* This program is intended to be started outside of gdb, and then
> > +   attached to by GDB.  */
> 
> gdb vs GDB.  Use uppercase in both cases.

Done in v2.
 
> > +
> > +#include <unistd.h>
> > +
> > +int
> > +main ()
> > +{
> > +  /* Don't run forever in case GDB crashes and DejaGNU fails to kill
> > +     this program.  */
> > +  alarm (10);
> > +
> > +  while (1)
> > +    ;
> 
> Please add some "usleep (1);" or some such in the loop to avoid hogging
> the CPU.

Done in v2.

> > +
> > +# The plain remote target can't do multiple inferiors.
> 
> Extended-remote can.  And, you start a separate gdbserver anyhow.  What happens if
> you remove this check?  You'd need to pass
> "set remote multiprocess-feature-packet off" and "maint set target-non-stop"
> via GDBFLAGS instead of as separate commands after starting gdb, but is there
> anything else?

No, there isn't.  I see your point.  I made this change in v2.

Thanks
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-03-29  8:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09  7:58 [PATCH] gdb/remote: use current_inferior in read_ptid if multi-process not supported Tankut Baris Aktemur
2022-02-22  7:33 ` Aktemur, Tankut Baris
2022-03-07  7:59 ` Aktemur, Tankut Baris
2022-03-07 16:03 ` Pedro Alves
2022-03-29  8:25   ` Aktemur, Tankut Baris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).