[gdb/tdep] Use pid to choose process 64/32-bitness

In a linux kernel mailing list discussion, it was mentioned that "gdb has
this odd thing where it takes the 64-bit vs 32-bit data for the whole process
from one thread, and picks the worst possible thread to do it (ie explicitly
not even the main thread, ...)" [1].

The picking of the thread is done here in
x86_linux_nat_target::read_description:
...
  /* GNU/Linux LWP ID's are process ID's.  */
  tid = inferior_ptid.lwp ();
  if (tid == 0)
    tid = inferior_ptid.pid (); /* Not a threaded program.  */
...

To understand what this code does, let's investigate a scenario in which
inferior_ptid.lwp () != inferior_ptid.pid ().

Say we start exec jit-attach-pie, identified with pid x.  The main thread
starts another thread that sleeps, and then the main thread waits for the
sleeping thread.  So we have two threads, identified with LWP IDs x and x+1:
...
PID  LWP  CMD
x    x    ./jit-attach-pie
x    x+1  ./jit-attach-pie
...
[ The thread with LWP x is known as the thread group leader. ]

When attaching to this exec using the pid, gdb does a stop_all_threads which
iterates over all the threads, first LWP x, and then LWP x+1.

So the state we arrive with at x86_linux_nat_target::read_description is:
...
(gdb) p inferior_ptid
$1 = {m_pid = x, m_lwp = x+1, m_tid = 0}
...
and consequently we probe 64/32-bitness from thread LWP x+1.

[ Note that this is different from when gdb doesn't attach but instead
launches the exec itself, in which case there's just one thread to begin with,
and consequently the probed thread is LWP x. ]

According to aforementioned remark, a better choice would have been the main
thread, that is, LWP x.

This patch implement that choice, by simply doing:
...
  tid = inferior_ptid.pid ();
...

The fact that gdb makes a per-process permanent choice for 64/32-bitness is a
problem in itself: each thread can be in either 64 or 32 bit mode, and change
forth and back.  That is a problem that this patch doesn't fix.

Now finally: why does this matter in the context of the linux kernel
discussion?  The discussion was related to a patch that exposed io_uring
threads to user-space.  This made it possible that one of those threads would
be picked out to select 64/32-bitness.  Given that such threads are atypical
user-space threads in the sense that they don't return to user-space and don't
have a userspace register state, reading their registers returns garbage, and
so it could f.i. occur that in a 64-bit process with all normal user-space
threads in 64-bit mode, the probing would return 32-bit.

It may be that this is worked-around on the kernel side by providing userspace
register state in those threads such that current gdb is happy.  Nevertheless,
it seems prudent to fix this on the gdb size as well.

Tested on x86_64-linux.

[1] https://lore.kernel.org/io-uring/CAHk-=wh0KoEZXPYMGkfkeVEerSCEF1AiCZSvz9TRrx=Kj74D+Q@mail.gmail.com/

gdb/ChangeLog:

2021-05-23  Tom de Vries  <tdevries@suse.de>

	PR tdep/27822
	* target.h (struct target_ops): Mention target_thread_architecture in
	read_description comment.
	* x86-linux-nat.c (x86_linux_nat_target::read_description): Use
	pid to determine if process is 64-bit or 32-bit.
	* aarch64-linux-nat.c (aarch64_linux_nat_target::read_description):
	Same.
	* ppc-linux-nat.c (ppc_linux_nat_target::read_description): Same.
        * riscv-linux-nat.c (riscv_linux_nat_target::read_description): Same.
	* s390-linux-nat.c (s390_linux_nat_target::read_description): Same.
	* arm-linux-nat.c (arm_linux_nat_target::read_description): Same.
	Likewise, use pid to determine if kernel supports reading VFP
	registers.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6e09089d179004acf201542a3f187f2130b75358..480d204232a6d054bd16361cc05fd8a744712c03 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2021-05-23  Tom de Vries  <tdevries@suse.de>
+
+       PR tdep/27822
+       * target.h (struct target_ops): Mention target_thread_architecture in
+       read_description comment.
+       * x86-linux-nat.c (x86_linux_nat_target::read_description): Use
+       pid to determine if process is 64-bit or 32-bit.
+       * aarch64-linux-nat.c (aarch64_linux_nat_target::read_description):
+       Same.
+       * ppc-linux-nat.c (ppc_linux_nat_target::read_description): Same.
+        * riscv-linux-nat.c (riscv_linux_nat_target::read_description): Same.
+       * s390-linux-nat.c (s390_linux_nat_target::read_description): Same.
+       * arm-linux-nat.c (arm_linux_nat_target::read_description): Same.
+       Likewise, use pid to determine if kernel supports reading VFP
+       registers.
+
 2021-05-22  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
 
        * main.c (enum cmdarg_kind): Fix option type comments for

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index ae8db2988c2b8b5eb9d59631448b4348facdbe0f..61224022f6aab16e74daa6115e69f613e934f2cb 100644 (file)
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -723,7 +723,7 @@ aarch64_linux_nat_target::read_description ()
   gdb_byte regbuf[ARM_VFP3_REGS_SIZE];
   struct iovec iovec;
 
-  tid = inferior_ptid.lwp ();
+  tid = inferior_ptid.pid ();
 
   iovec.iov_base = regbuf;
   iovec.iov_len = ARM_VFP3_REGS_SIZE;

diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 662dade0a122f8adf25da91577b1de375df7785b..880ac0da0440e8ed34939ff6be5e9c88ea472e71 100644 (file)
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -537,7 +537,7 @@ arm_linux_nat_target::read_description ()
     {
       elf_gregset_t gpregs;
       struct iovec iov;
-      int tid = inferior_ptid.lwp ();
+      int tid = inferior_ptid.pid ();
 
       iov.iov_base = &gpregs;
       iov.iov_len = sizeof (gpregs);
@@ -556,7 +556,7 @@ arm_linux_nat_target::read_description ()
     {
       /* Make sure that the kernel supports reading VFP registers.  Support was
         added in 2.6.30.  */
-      int pid = inferior_ptid.lwp ();
+      int pid = inferior_ptid.pid ();
       errno = 0;
       char *buf = (char *) alloca (ARM_VFP3_REGS_SIZE);
       if (ptrace (PTRACE_GETVFPREGS, pid, 0, buf) < 0 && errno == EIO)

diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index 171f5b386fa8d3848d0875372cb7a18689a61874..06a30efeaef4908c863269f57844e1ff832575aa 100644 (file)
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1946,9 +1946,7 @@ ppc_linux_nat_target::auxv_parse (gdb_byte **readptr,
 const struct target_desc *
 ppc_linux_nat_target::read_description ()
 {
-  int tid = inferior_ptid.lwp ();
-  if (tid == 0)
-    tid = inferior_ptid.pid ();
+  int tid = inferior_ptid.pid ();
 
   if (have_ptrace_getsetevrregs)
     {

diff --git a/gdb/riscv-linux-nat.c b/gdb/riscv-linux-nat.c
index 04bf46b3bb182660110e5f5740d1b3c3a0dd2cb5..c0f5a27a37eb24e2dba305166a82f319cd4ae8e7 100644 (file)
--- a/gdb/riscv-linux-nat.c
+++ b/gdb/riscv-linux-nat.c
@@ -202,7 +202,7 @@ const struct target_desc *
 riscv_linux_nat_target::read_description ()
 {
   const struct riscv_gdbarch_features features
-    = riscv_linux_read_features (inferior_ptid.lwp ());
+    = riscv_linux_read_features (inferior_ptid.pid ());
   return riscv_lookup_target_description (features);
 }
 

diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 41b50ce4800346bdbc301d1114ab1f4976f413c2..8f6eb61505bd1ec5345f585284edc9e2bddc564b 100644 (file)
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -988,7 +988,7 @@ s390_linux_nat_target::auxv_parse (gdb_byte **readptr,
 const struct target_desc *
 s390_linux_nat_target::read_description ()
 {
-  int tid = s390_inferior_tid ();
+  int tid = inferior_ptid.pid ();
 
   have_regset_last_break
     = check_regset (tid, NT_S390_LAST_BREAK, 8);

diff --git a/gdb/target.h b/gdb/target.h
index d867a58e2a8b0196d67920934cda3bad6c178484..e22f9038197c55d93dc1b10db3926aa30c613a07 100644 (file)
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -838,10 +838,17 @@ struct target_ops
     virtual void flash_done ()
       TARGET_DEFAULT_NORETURN (tcomplain ());
 
-    /* Describe the architecture-specific features of this target.  If
-       OPS doesn't have a description, this should delegate to the
-       "beneath" target.  Returns the description found, or NULL if no
-       description was available.  */
+    /* Describe the architecture-specific features of the current
+       inferior.
+
+       Returns the description found, or nullptr if no description was
+       available.
+
+       If some target features differ between threads, the description
+       returned by read_description (and the resulting gdbarch) won't
+       accurately describe all threads.  In this case, the
+       thread_architecture method can be used to obtain gdbarches that
+       accurately describe each thread.  */
     virtual const struct target_desc *read_description ()
         TARGET_DEFAULT_RETURN (NULL);
 

diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c
index 85c7f0ddc94b510a43246a9a6582611c0009588c..adea1ad00922668f72527e45a3c8838e93076efb 100644 (file)
--- a/gdb/x86-linux-nat.c
+++ b/gdb/x86-linux-nat.c
@@ -113,10 +113,7 @@ x86_linux_nat_target::read_description ()
   static uint64_t xcr0;
   uint64_t xcr0_features_bits;
 
-  /* GNU/Linux LWP ID's are process ID's.  */
-  tid = inferior_ptid.lwp ();
-  if (tid == 0)
-    tid = inferior_ptid.pid (); /* Not a threaded program.  */
+  tid = inferior_ptid.pid ();
 
 #ifdef __x86_64__
   {