[PATCH gdb-10-branch candidate] gdb: set current thread in sparc_{fetch, collect}_inferior_registers (PR gdb/27147)

[PATCH gdb-10-branch candidate] gdb: set current thread in sparc_{fetch, collect}_inferior_registers (PR gdb/27147)

[Simon Marchi]

PR 27147 shows that on sparc64, GDB is unable to properly unwind:

Expected result (from GDB 9.2):

    #0  0x0000000000108de4 in puts ()
    #1  0x0000000000100950 in hello () at gdb-test.c:4
    #2  0x0000000000100968 in main () at gdb-test.c:8

Actual result (from GDB latest git):

    #0  0x0000000000108de4 in puts ()
    #1  0x0000000000100950 in hello () at gdb-test.c:4
    Backtrace stopped: previous frame inner to this frame (corrupt stack?)

The first failing commit is 5b6d1e4fa4fc ("Multi-target support").  The cause
of the change in behavior is due to (thanks for Andrew Burgess for finding
this):

 - inferior_ptid is no longer set on entry of target_ops::wait, whereas
   it was set to something valid previously
 - deep down in linux_nat_target::wait (see stack trace below), we fetch
   the registers of the event thread
 - on sparc64, fetching registers involves reading memory (in
   sparc_supply_rwindow, see stack trace below)
 - reading memory (target_ops::xfer_partial) relies on inferior_ptid
   being set to the thread from which we want to read memory

This is where things go wrong:

    #0  linux_nat_target::xfer_partial (this=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7feffe3b000 "", writebuf=0x0, offset=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3697
    #1  0x00000100007f5b10 in raw_memory_xfer_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, readbuf=0x7feffe3b000 "", writebuf=0x0, memaddr=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:912
    #2  0x00000100007f60e8 in memory_xfer_partial_1 (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, readbuf=0x7feffe3b000 "", writebuf=0x0, memaddr=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1043
    #3  0x00000100007f61b4 in memory_xfer_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, readbuf=0x7feffe3b000 "", writebuf=0x0, memaddr=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1072
    #4  0x00000100007f6538 in target_xfer_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7feffe3b000 "", writebuf=0x0, offset=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1129
    #5  0x00000100007f7094 in target_read_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7feffe3b000 "", offset=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1375
    #6  0x00000100007f721c in target_read (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7feffe3b000 "", offset=8791798050744, len=8) at /home/simark/src/binutils-gdb/gdb/target.c:1415
    #7  0x00000100007f69d4 in target_read_memory (memaddr=8791798050744, myaddr=0x7feffe3b000 "", len=8) at /home/simark/src/binutils-gdb/gdb/target.c:1218
    #8  0x0000010000758520 in sparc_supply_rwindow (regcache=0x10000fea4f0, sp=8791798050736, regnum=-1) at /home/simark/src/binutils-gdb/gdb/sparc-tdep.c:1960
    #9  0x000001000076208c in sparc64_supply_gregset (gregmap=0x10000be3190 <sparc64_linux_ptrace_gregmap>, regcache=0x10000fea4f0, regnum=-1, gregs=0x7feffe3b230) at /home/simark/src/binutils-gdb/gdb/sparc64-tdep.c:1974
    #10 0x0000010000751b64 in sparc_fetch_inferior_registers (regcache=0x10000fea4f0, regnum=80) at /home/simark/src/binutils-gdb/gdb/sparc-nat.c:170
    #11 0x0000010000759d68 in sparc64_linux_nat_target::fetch_registers (this=0x10000fa2c40 <the_sparc64_linux_nat_target>, regcache=0x10000fea4f0, regnum=80) at /home/simark/src/binutils-gdb/gdb/sparc64-linux-nat.c:38
    #12 0x00000100008146ec in target_fetch_registers (regcache=0x10000fea4f0, regno=80) at /home/simark/src/binutils-gdb/gdb/target.c:3287
    #13 0x00000100006a8c5c in regcache::raw_update (this=0x10000fea4f0, regnum=80) at /home/simark/src/binutils-gdb/gdb/regcache.c:584
    #14 0x00000100006a8d94 in readable_regcache::raw_read (this=0x10000fea4f0, regnum=80, buf=0x7feffe3b7c0 "") at /home/simark/src/binutils-gdb/gdb/regcache.c:598
    #15 0x00000100006a93b8 in readable_regcache::cooked_read (this=0x10000fea4f0, regnum=80, buf=0x7feffe3b7c0 "") at /home/simark/src/binutils-gdb/gdb/regcache.c:690
    #16 0x00000100006b288c in readable_regcache::cooked_read<unsigned long, void> (this=0x10000fea4f0, regnum=80, val=0x7feffe3b948) at /home/simark/src/binutils-gdb/gdb/regcache.c:777
    #17 0x00000100006a9b44 in regcache_cooked_read_unsigned (regcache=0x10000fea4f0, regnum=80, val=0x7feffe3b948) at /home/simark/src/binutils-gdb/gdb/regcache.c:791
    #18 0x00000100006abf3c in regcache_read_pc (regcache=0x10000fea4f0) at /home/simark/src/binutils-gdb/gdb/regcache.c:1295
    #19 0x0000010000507920 in save_stop_reason (lp=0x10000fc5b10) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:2612
    #20 0x00000100005095a4 in linux_nat_filter_event (lwpid=520983, status=1407) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3050
    #21 0x0000010000509f9c in linux_nat_wait_1 (ptid=..., ourstatus=0x7feffe3c8f0, target_options=...) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3194
    #22 0x000001000050b1d0 in linux_nat_target::wait (this=0x10000fa2c40 <the_sparc64_linux_nat_target>, ptid=..., ourstatus=0x7feffe3c8f0, target_options=...) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3432
    #23 0x00000100007f8ac0 in target_wait (ptid=..., status=0x7feffe3c8f0, options=...) at /home/simark/src/binutils-gdb/gdb/target.c:2000
    #24 0x00000100004ac17c in do_target_wait_1 (inf=0x1000116d280, ptid=..., status=0x7feffe3c8f0, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3464
    #25 0x00000100004ac3b8 in operator() (__closure=0x7feffe3c678, inf=0x1000116d280) at /home/simark/src/binutils-gdb/gdb/infrun.c:3527
    #26 0x00000100004ac7cc in do_target_wait (wait_ptid=..., ecs=0x7feffe3c8c8, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3540
    #27 0x00000100004ad8c4 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:3880
    #28 0x0000010000485568 in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:42
    #29 0x000001000050d394 in handle_target_event (error=0, client_data=0x0) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:4060
    #30 0x0000010000ab5c8c in handle_file_event (file_ptr=0x10001207270, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:575
    #31 0x0000010000ab6334 in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:701
    #32 0x0000010000ab487c in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
    #33 0x0000010000542668 in start_event_loop () at /home/simark/src/binutils-gdb/gdb/main.c:348
    #34 0x000001000054287c in captured_command_loop () at /home/simark/src/binutils-gdb/gdb/main.c:408
    #35 0x0000010000544e84 in captured_main (data=0x7feffe3d188) at /home/simark/src/binutils-gdb/gdb/main.c:1242
    #36 0x0000010000544f2c in gdb_main (args=0x7feffe3d188) at /home/simark/src/binutils-gdb/gdb/main.c:1257
    #37 0x00000100000c1f14 in main (argc=4, argv=0x7feffe3d548) at /home/simark/src/binutils-gdb/gdb/gdb.c:32

There is a target_read_memory call in sparc_supply_rwindow, whose return
value is not checked.  That call fails, because inferior_ptid does not
contain a valid ptid, and uninitialized buffer contents is used.
Ultimately it results in a corrupt stop_pc.

target_ops::fetch_registers can be (and should remain, in my opinion)
independent of inferior_ptid, because the ptid of the thread from which
to fetch registers can be obtained from the regcache.  In other words,
implementations of target_ops::fetch_registers should not rely on
inferior_ptid having a sensible value on entry.

The sparc64_linux_nat_target::fetch_registers case is special, because it calls
a target method that is dependent on the inferior_ptid value
(target_read_inferior, and ultimately target_ops::xfer_partial).  So I would
say it's the responsibility of sparc64_linux_nat_target::fetch_registers to set
up inferior_ptid correctly prior to calling target_read_inferior.

This patch makes sparc64_linux_nat_target::fetch_registers (and
store_registers, since it works the same) temporarily set inferior_ptid.  If we
ever make target_ops::xfer_partial independent of inferior_ptid, setting
inferior_ptid won't be necessary, we'll simply pass down the ptid as a
parameter in some way.

I chose to set/restore inferior_ptid in sparc_fetch_inferior_registers, because
I am not convinced that doing so in an inner location (in sparc_supply_rwindow
for instance) would always be correct.  We have access to the ptid in
sparc_supply_rwindow (from the regcache), so we _could_ set inferior_ptid
there.  However, I don't want to just set inferior_ptid, as that would make it
not desync'ed with `current_thread ()` and `current_inferior ()`.  It's
preferable to use switch_to_thread instead, as that switches all the global
"current" stuff in a coherent way.  But doing so requires a `thread_info *`,
and getting a `thread_info *` from a ptid requires a `process_stratum_target
*`.  We could use `current_inferior()->process_target()` in
sparc_supply_rwindow for this (using target_read_memory uses the current
inferior's target stack anyway).  However, sparc_supply_rwindow is also used in
the context of BSD uthreads, where a thread stratum target defines threads.  I
presume the ptid in the regcache would be the ptid of the uthread, defined by
the thread stratum target (bsd_uthread_target).  Using
`current_inferior()->process_target()` would look up a ptid defined by the
thread stratum target using the process stratum target.  I don't think it would
give good results.  So I prefer playing it safe and looking up the thread
earlier, in sparc_fetch_inferior_registers.

I added some assertions (in sparc_supply_rwindow and others) to verify
that the regcache's ptid matches inferior_ptid.  That verifies that the
caller has properly set the correct global context.  This would have
caught (though a failed assertion) the current problem.

gdb/ChangeLog:

	PR gdb/27147
	* sparc-nat.h (sparc_fetch_inferior_registers): Add
	process_stratum_target parameter,
	sparc_store_inferior_registers): update callers.
	* sparc-nat.c (sparc_fetch_inferior_registers,
	sparc_store_inferior_registers): Add process_stratum_target
	parameter.  Switch current thread before calling
	sparc_supply_gregset / sparc_collect_rwindow.
	(sparc_store_inferior_registers): Likewise.
	* sparc-obsd-tdep.c (sparc32obsd_supply_uthread): Add assertion.
	(sparc32obsd_collect_uthread): Likewise.
	* sparc-tdep.c (sparc_supply_rwindow, sparc_collect_rwindow):
	Add assertion.
	* sparc64-obsd-tdep.c (sparc64obsd_collect_uthread,
	sparc64obsd_supply_uthread): Add assertion.

Change-Id: I16c658cd70896cea604516714f7e2428fbaf4301
---
 gdb/sparc-nat.c         | 19 +++++++++++++++++--
 gdb/sparc-nat.h         | 10 ++++++----
 gdb/sparc-obsd-tdep.c   |  7 +++++++
 gdb/sparc-tdep.c        |  6 ++++++
 gdb/sparc64-linux-nat.c |  4 ++--
 gdb/sparc64-obsd-tdep.c |  7 +++++++
 6 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/gdb/sparc-nat.c b/gdb/sparc-nat.c
index bc3afce20277..fa3b32cee184 100644
--- a/gdb/sparc-nat.c
+++ b/gdb/sparc-nat.c
@@ -147,7 +147,8 @@ sparc32_fpregset_supplies_p (struct gdbarch *gdbarch, int regnum)
    for all registers (including the floating-point registers).  */
 
 void
-sparc_fetch_inferior_registers (struct regcache *regcache, int regnum)
+sparc_fetch_inferior_registers (process_stratum_target *proc_target,
+				regcache *regcache, int regnum)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   ptid_t ptid = regcache->ptid ();
@@ -167,6 +168,12 @@ sparc_fetch_inferior_registers (struct regcache *regcache, int regnum)
       if (gdb_ptrace (PTRACE_GETREGS, ptid, (PTRACE_TYPE_ARG3) &regs) == -1)
 	perror_with_name (_("Couldn't get registers"));
 
+      /* Deep down, sparc_supply_rwindow reads memory, so needs the global
+	 thread context to be set.  */
+      thread_info *thread = find_thread_ptid (proc_target, ptid);
+      scoped_restore_current_thread restore_thread;
+      switch_to_thread (thread);
+
       sparc_supply_gregset (sparc_gregmap, regcache, -1, &regs);
       if (regnum != -1)
 	return;
@@ -184,7 +191,8 @@ sparc_fetch_inferior_registers (struct regcache *regcache, int regnum)
 }
 
 void
-sparc_store_inferior_registers (struct regcache *regcache, int regnum)
+sparc_store_inferior_registers (process_stratum_target *proc_target,
+				regcache *regcache, int regnum)
 {
   struct gdbarch *gdbarch = regcache->arch ();
   ptid_t ptid = regcache->ptid ();
@@ -208,6 +216,13 @@ sparc_store_inferior_registers (struct regcache *regcache, int regnum)
 	  ULONGEST sp;
 
 	  regcache_cooked_read_unsigned (regcache, SPARC_SP_REGNUM, &sp);
+
+	  /* Deep down, sparc_collect_rwindow writes memory, so needs the global
+	     thread context to be set.  */
+	  thread_info *thread = find_thread_ptid (proc_target, ptid);
+	  scoped_restore_current_thread restore_thread;
+	  switch_to_thread (thread);
+
 	  sparc_collect_rwindow (regcache, sp, regnum);
 	}
 
diff --git a/gdb/sparc-nat.h b/gdb/sparc-nat.h
index 196bf1f2307e..9d0c24f731f6 100644
--- a/gdb/sparc-nat.h
+++ b/gdb/sparc-nat.h
@@ -41,8 +41,10 @@ extern int (*sparc_fpregset_supplies_p) (struct gdbarch *gdbarch, int);
 extern int sparc32_gregset_supplies_p (struct gdbarch *gdbarch, int regnum);
 extern int sparc32_fpregset_supplies_p (struct gdbarch *gdbarch, int regnum);
 
-extern void sparc_fetch_inferior_registers (struct regcache *, int);
-extern void sparc_store_inferior_registers (struct regcache *, int);
+extern void sparc_fetch_inferior_registers (process_stratum_target *proc_target,
+					    regcache *, int);
+extern void sparc_store_inferior_registers (process_stratum_target *proc_target,
+					    regcache *, int);
 
 extern target_xfer_status sparc_xfer_wcookie (enum target_object object,
 					      const char *annex,
@@ -59,10 +61,10 @@ template<typename BaseTarget>
 struct sparc_target : public BaseTarget
 {
   void fetch_registers (struct regcache *regcache, int regnum) override
-  { sparc_fetch_inferior_registers (regcache, regnum); }
+  { sparc_fetch_inferior_registers (this, regcache, regnum); }
 
   void store_registers (struct regcache *regcache, int regnum) override
-  { sparc_store_inferior_registers (regcache, regnum); }
+  { sparc_store_inferior_registers (this, regcache, regnum); }
 
   enum target_xfer_status xfer_partial (enum target_object object,
 					const char *annex,
diff --git a/gdb/sparc-obsd-tdep.c b/gdb/sparc-obsd-tdep.c
index 6cd9227f8aed..e198e9702cdb 100644
--- a/gdb/sparc-obsd-tdep.c
+++ b/gdb/sparc-obsd-tdep.c
@@ -25,6 +25,7 @@
 #include "regcache.h"
 #include "symtab.h"
 #include "trad-frame.h"
+#include "inferior.h"
 
 #include "obsd-tdep.h"
 #include "sparc-tdep.h"
@@ -158,6 +159,9 @@ sparc32obsd_supply_uthread (struct regcache *regcache,
   CORE_ADDR fp, fp_addr = addr + SPARC32OBSD_UTHREAD_FP_OFFSET;
   gdb_byte buf[4];
 
+  /* This function calls functions that depend on the global current thread.  */
+  gdb_assert (regcache->ptid () == inferior_ptid);
+
   gdb_assert (regnum >= -1);
 
   fp = read_memory_unsigned_integer (fp_addr, 4, byte_order);
@@ -203,6 +207,9 @@ sparc32obsd_collect_uthread(const struct regcache *regcache,
   CORE_ADDR sp;
   gdb_byte buf[4];
 
+  /* This function calls functions that depend on the global current thread.  */
+  gdb_assert (regcache->ptid () == inferior_ptid);
+
   gdb_assert (regnum >= -1);
 
   if (regnum == SPARC_SP_REGNUM || regnum == -1)
diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c
index 4f9c679b55cd..34f22879737a 100644
--- a/gdb/sparc-tdep.c
+++ b/gdb/sparc-tdep.c
@@ -1948,6 +1948,9 @@ sparc_supply_rwindow (struct regcache *regcache, CORE_ADDR sp, int regnum)
   gdb_byte buf[8];
   int i;
 
+  /* This function calls functions that depend on the global current thread.  */
+  gdb_assert (regcache->ptid () == inferior_ptid);
+
   if (sp & 1)
     {
       /* Registers are 64-bit.  */
@@ -2022,6 +2025,9 @@ sparc_collect_rwindow (const struct regcache *regcache,
   gdb_byte buf[8];
   int i;
 
+  /* This function calls functions that depend on the global current thread.  */
+  gdb_assert (regcache->ptid () == inferior_ptid);
+
   if (sp & 1)
     {
       /* Registers are 64-bit.  */
diff --git a/gdb/sparc64-linux-nat.c b/gdb/sparc64-linux-nat.c
index ad2cc35bd70f..b741d0216c32 100644
--- a/gdb/sparc64-linux-nat.c
+++ b/gdb/sparc64-linux-nat.c
@@ -35,10 +35,10 @@ class sparc64_linux_nat_target final : public linux_nat_target
 public:
   /* Add our register access methods.  */
   void fetch_registers (struct regcache *regcache, int regnum) override
-  { sparc_fetch_inferior_registers (regcache, regnum); }
+  { sparc_fetch_inferior_registers (this, regcache, regnum); }
 
   void store_registers (struct regcache *regcache, int regnum) override
-  { sparc_store_inferior_registers (regcache, regnum); }
+  { sparc_store_inferior_registers (this, regcache, regnum); }
 
   /* Override linux_nat_target low methods.  */
 
diff --git a/gdb/sparc64-obsd-tdep.c b/gdb/sparc64-obsd-tdep.c
index 9e678d268ad7..cf3138a7ab14 100644
--- a/gdb/sparc64-obsd-tdep.c
+++ b/gdb/sparc64-obsd-tdep.c
@@ -27,6 +27,7 @@
 #include "symtab.h"
 #include "objfiles.h"
 #include "trad-frame.h"
+#include "inferior.h"
 
 #include "obsd-tdep.h"
 #include "sparc64-tdep.h"
@@ -328,6 +329,9 @@ sparc64obsd_supply_uthread (struct regcache *regcache,
   CORE_ADDR fp, fp_addr = addr + SPARC64OBSD_UTHREAD_FP_OFFSET;
   gdb_byte buf[8];
 
+  /* This function calls functions that depend on the global current thread.  */
+  gdb_assert (regcache->ptid () == inferior_ptid);
+
   gdb_assert (regnum >= -1);
 
   fp = read_memory_unsigned_integer (fp_addr, 8, byte_order);
@@ -373,6 +377,9 @@ sparc64obsd_collect_uthread(const struct regcache *regcache,
   CORE_ADDR sp;
   gdb_byte buf[8];
 
+  /* This function calls functions that depend on the global current thread.  */
+  gdb_assert (regcache->ptid () == inferior_ptid);
+
   gdb_assert (regnum >= -1);
 
   if (regnum == SPARC_SP_REGNUM || regnum == -1)
-- 
2.30.1

Re: [PATCH gdb-10-branch candidate] gdb: set current thread in sparc_{fetch,collect}_inferior_registers (PR gdb/27147)

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-03-03 19:49:17 -0500]:

> PR 27147 shows that on sparc64, GDB is unable to properly unwind:
> 
> Expected result (from GDB 9.2):
> 
>     #0  0x0000000000108de4 in puts ()
>     #1  0x0000000000100950 in hello () at gdb-test.c:4
>     #2  0x0000000000100968 in main () at gdb-test.c:8
> 
> Actual result (from GDB latest git):
> 
>     #0  0x0000000000108de4 in puts ()
>     #1  0x0000000000100950 in hello () at gdb-test.c:4
>     Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> 
> The first failing commit is 5b6d1e4fa4fc ("Multi-target support").  The cause
> of the change in behavior is due to (thanks for Andrew Burgess for finding
> this):
> 
>  - inferior_ptid is no longer set on entry of target_ops::wait, whereas
>    it was set to something valid previously
>  - deep down in linux_nat_target::wait (see stack trace below), we fetch
>    the registers of the event thread
>  - on sparc64, fetching registers involves reading memory (in
>    sparc_supply_rwindow, see stack trace below)
>  - reading memory (target_ops::xfer_partial) relies on inferior_ptid
>    being set to the thread from which we want to read memory
> 
> This is where things go wrong:
> 
>     #0  linux_nat_target::xfer_partial (this=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7feffe3b000 "", writebuf=0x0, offset=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3697
>     #1  0x00000100007f5b10 in raw_memory_xfer_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, readbuf=0x7feffe3b000 "", writebuf=0x0, memaddr=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:912
>     #2  0x00000100007f60e8 in memory_xfer_partial_1 (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, readbuf=0x7feffe3b000 "", writebuf=0x0, memaddr=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1043
>     #3  0x00000100007f61b4 in memory_xfer_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, readbuf=0x7feffe3b000 "", writebuf=0x0, memaddr=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1072
>     #4  0x00000100007f6538 in target_xfer_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7feffe3b000 "", writebuf=0x0, offset=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1129
>     #5  0x00000100007f7094 in target_read_partial (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7feffe3b000 "", offset=8791798050744, len=8, xfered_len=0x7feffe3ae88) at /home/simark/src/binutils-gdb/gdb/target.c:1375
>     #6  0x00000100007f721c in target_read (ops=0x10000fa2c40 <the_sparc64_linux_nat_target>, object=TARGET_OBJECT_MEMORY, annex=0x0, buf=0x7feffe3b000 "", offset=8791798050744, len=8) at /home/simark/src/binutils-gdb/gdb/target.c:1415
>     #7  0x00000100007f69d4 in target_read_memory (memaddr=8791798050744, myaddr=0x7feffe3b000 "", len=8) at /home/simark/src/binutils-gdb/gdb/target.c:1218
>     #8  0x0000010000758520 in sparc_supply_rwindow (regcache=0x10000fea4f0, sp=8791798050736, regnum=-1) at /home/simark/src/binutils-gdb/gdb/sparc-tdep.c:1960
>     #9  0x000001000076208c in sparc64_supply_gregset (gregmap=0x10000be3190 <sparc64_linux_ptrace_gregmap>, regcache=0x10000fea4f0, regnum=-1, gregs=0x7feffe3b230) at /home/simark/src/binutils-gdb/gdb/sparc64-tdep.c:1974
>     #10 0x0000010000751b64 in sparc_fetch_inferior_registers (regcache=0x10000fea4f0, regnum=80) at /home/simark/src/binutils-gdb/gdb/sparc-nat.c:170
>     #11 0x0000010000759d68 in sparc64_linux_nat_target::fetch_registers (this=0x10000fa2c40 <the_sparc64_linux_nat_target>, regcache=0x10000fea4f0, regnum=80) at /home/simark/src/binutils-gdb/gdb/sparc64-linux-nat.c:38
>     #12 0x00000100008146ec in target_fetch_registers (regcache=0x10000fea4f0, regno=80) at /home/simark/src/binutils-gdb/gdb/target.c:3287
>     #13 0x00000100006a8c5c in regcache::raw_update (this=0x10000fea4f0, regnum=80) at /home/simark/src/binutils-gdb/gdb/regcache.c:584
>     #14 0x00000100006a8d94 in readable_regcache::raw_read (this=0x10000fea4f0, regnum=80, buf=0x7feffe3b7c0 "") at /home/simark/src/binutils-gdb/gdb/regcache.c:598
>     #15 0x00000100006a93b8 in readable_regcache::cooked_read (this=0x10000fea4f0, regnum=80, buf=0x7feffe3b7c0 "") at /home/simark/src/binutils-gdb/gdb/regcache.c:690
>     #16 0x00000100006b288c in readable_regcache::cooked_read<unsigned long, void> (this=0x10000fea4f0, regnum=80, val=0x7feffe3b948) at /home/simark/src/binutils-gdb/gdb/regcache.c:777
>     #17 0x00000100006a9b44 in regcache_cooked_read_unsigned (regcache=0x10000fea4f0, regnum=80, val=0x7feffe3b948) at /home/simark/src/binutils-gdb/gdb/regcache.c:791
>     #18 0x00000100006abf3c in regcache_read_pc (regcache=0x10000fea4f0) at /home/simark/src/binutils-gdb/gdb/regcache.c:1295
>     #19 0x0000010000507920 in save_stop_reason (lp=0x10000fc5b10) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:2612
>     #20 0x00000100005095a4 in linux_nat_filter_event (lwpid=520983, status=1407) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3050
>     #21 0x0000010000509f9c in linux_nat_wait_1 (ptid=..., ourstatus=0x7feffe3c8f0, target_options=...) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3194
>     #22 0x000001000050b1d0 in linux_nat_target::wait (this=0x10000fa2c40 <the_sparc64_linux_nat_target>, ptid=..., ourstatus=0x7feffe3c8f0, target_options=...) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:3432
>     #23 0x00000100007f8ac0 in target_wait (ptid=..., status=0x7feffe3c8f0, options=...) at /home/simark/src/binutils-gdb/gdb/target.c:2000
>     #24 0x00000100004ac17c in do_target_wait_1 (inf=0x1000116d280, ptid=..., status=0x7feffe3c8f0, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3464
>     #25 0x00000100004ac3b8 in operator() (__closure=0x7feffe3c678, inf=0x1000116d280) at /home/simark/src/binutils-gdb/gdb/infrun.c:3527
>     #26 0x00000100004ac7cc in do_target_wait (wait_ptid=..., ecs=0x7feffe3c8c8, options=...) at /home/simark/src/binutils-gdb/gdb/infrun.c:3540
>     #27 0x00000100004ad8c4 in fetch_inferior_event () at /home/simark/src/binutils-gdb/gdb/infrun.c:3880
>     #28 0x0000010000485568 in inferior_event_handler (event_type=INF_REG_EVENT) at /home/simark/src/binutils-gdb/gdb/inf-loop.c:42
>     #29 0x000001000050d394 in handle_target_event (error=0, client_data=0x0) at /home/simark/src/binutils-gdb/gdb/linux-nat.c:4060
>     #30 0x0000010000ab5c8c in handle_file_event (file_ptr=0x10001207270, ready_mask=1) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:575
>     #31 0x0000010000ab6334 in gdb_wait_for_event (block=0) at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:701
>     #32 0x0000010000ab487c in gdb_do_one_event () at /home/simark/src/binutils-gdb/gdbsupport/event-loop.cc:212
>     #33 0x0000010000542668 in start_event_loop () at /home/simark/src/binutils-gdb/gdb/main.c:348
>     #34 0x000001000054287c in captured_command_loop () at /home/simark/src/binutils-gdb/gdb/main.c:408
>     #35 0x0000010000544e84 in captured_main (data=0x7feffe3d188) at /home/simark/src/binutils-gdb/gdb/main.c:1242
>     #36 0x0000010000544f2c in gdb_main (args=0x7feffe3d188) at /home/simark/src/binutils-gdb/gdb/main.c:1257
>     #37 0x00000100000c1f14 in main (argc=4, argv=0x7feffe3d548) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
> 
> There is a target_read_memory call in sparc_supply_rwindow, whose return
> value is not checked.  That call fails, because inferior_ptid does not
> contain a valid ptid, and uninitialized buffer contents is used.
> Ultimately it results in a corrupt stop_pc.
> 
> target_ops::fetch_registers can be (and should remain, in my opinion)
> independent of inferior_ptid, because the ptid of the thread from which
> to fetch registers can be obtained from the regcache.  In other words,
> implementations of target_ops::fetch_registers should not rely on
> inferior_ptid having a sensible value on entry.
> 
> The sparc64_linux_nat_target::fetch_registers case is special, because it calls
> a target method that is dependent on the inferior_ptid value
> (target_read_inferior, and ultimately target_ops::xfer_partial).  So I would
> say it's the responsibility of sparc64_linux_nat_target::fetch_registers to set
> up inferior_ptid correctly prior to calling target_read_inferior.
> 
> This patch makes sparc64_linux_nat_target::fetch_registers (and
> store_registers, since it works the same) temporarily set inferior_ptid.  If we
> ever make target_ops::xfer_partial independent of inferior_ptid, setting
> inferior_ptid won't be necessary, we'll simply pass down the ptid as a
> parameter in some way.
> 
> I chose to set/restore inferior_ptid in sparc_fetch_inferior_registers, because
> I am not convinced that doing so in an inner location (in sparc_supply_rwindow
> for instance) would always be correct.  We have access to the ptid in
> sparc_supply_rwindow (from the regcache), so we _could_ set inferior_ptid
> there.  However, I don't want to just set inferior_ptid, as that would make it
> not desync'ed with `current_thread ()` and `current_inferior ()`.  It's
> preferable to use switch_to_thread instead, as that switches all the global
> "current" stuff in a coherent way.  But doing so requires a `thread_info *`,
> and getting a `thread_info *` from a ptid requires a `process_stratum_target
> *`.  We could use `current_inferior()->process_target()` in
> sparc_supply_rwindow for this (using target_read_memory uses the current
> inferior's target stack anyway).  However, sparc_supply_rwindow is also used in
> the context of BSD uthreads, where a thread stratum target defines threads.  I
> presume the ptid in the regcache would be the ptid of the uthread, defined by
> the thread stratum target (bsd_uthread_target).  Using
> `current_inferior()->process_target()` would look up a ptid defined by the
> thread stratum target using the process stratum target.  I don't think it would
> give good results.  So I prefer playing it safe and looking up the thread
> earlier, in sparc_fetch_inferior_registers.
> 
> I added some assertions (in sparc_supply_rwindow and others) to verify
> that the regcache's ptid matches inferior_ptid.  That verifies that the
> caller has properly set the correct global context.  This would have
> caught (though a failed assertion) the current problem.
> 
> gdb/ChangeLog:
> 
> 	PR gdb/27147
> 	* sparc-nat.h (sparc_fetch_inferior_registers): Add
> 	process_stratum_target parameter,
> 	sparc_store_inferior_registers): update callers.
> 	* sparc-nat.c (sparc_fetch_inferior_registers,
> 	sparc_store_inferior_registers): Add process_stratum_target
> 	parameter.  Switch current thread before calling
> 	sparc_supply_gregset / sparc_collect_rwindow.
> 	(sparc_store_inferior_registers): Likewise.
> 	* sparc-obsd-tdep.c (sparc32obsd_supply_uthread): Add assertion.
> 	(sparc32obsd_collect_uthread): Likewise.
> 	* sparc-tdep.c (sparc_supply_rwindow, sparc_collect_rwindow):
> 	Add assertion.
> 	* sparc64-obsd-tdep.c (sparc64obsd_collect_uthread,
> 	sparc64obsd_supply_uthread): Add assertion.

LGTM.

Thanks,
Andrew


> 
> Change-Id: I16c658cd70896cea604516714f7e2428fbaf4301
> ---
>  gdb/sparc-nat.c         | 19 +++++++++++++++++--
>  gdb/sparc-nat.h         | 10 ++++++----
>  gdb/sparc-obsd-tdep.c   |  7 +++++++
>  gdb/sparc-tdep.c        |  6 ++++++
>  gdb/sparc64-linux-nat.c |  4 ++--
>  gdb/sparc64-obsd-tdep.c |  7 +++++++
>  6 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/sparc-nat.c b/gdb/sparc-nat.c
> index bc3afce20277..fa3b32cee184 100644
> --- a/gdb/sparc-nat.c
> +++ b/gdb/sparc-nat.c
> @@ -147,7 +147,8 @@ sparc32_fpregset_supplies_p (struct gdbarch *gdbarch, int regnum)
>     for all registers (including the floating-point registers).  */
>  
>  void
> -sparc_fetch_inferior_registers (struct regcache *regcache, int regnum)
> +sparc_fetch_inferior_registers (process_stratum_target *proc_target,
> +				regcache *regcache, int regnum)
>  {
>    struct gdbarch *gdbarch = regcache->arch ();
>    ptid_t ptid = regcache->ptid ();
> @@ -167,6 +168,12 @@ sparc_fetch_inferior_registers (struct regcache *regcache, int regnum)
>        if (gdb_ptrace (PTRACE_GETREGS, ptid, (PTRACE_TYPE_ARG3) &regs) == -1)
>  	perror_with_name (_("Couldn't get registers"));
>  
> +      /* Deep down, sparc_supply_rwindow reads memory, so needs the global
> +	 thread context to be set.  */
> +      thread_info *thread = find_thread_ptid (proc_target, ptid);
> +      scoped_restore_current_thread restore_thread;
> +      switch_to_thread (thread);
> +
>        sparc_supply_gregset (sparc_gregmap, regcache, -1, &regs);
>        if (regnum != -1)
>  	return;
> @@ -184,7 +191,8 @@ sparc_fetch_inferior_registers (struct regcache *regcache, int regnum)
>  }
>  
>  void
> -sparc_store_inferior_registers (struct regcache *regcache, int regnum)
> +sparc_store_inferior_registers (process_stratum_target *proc_target,
> +				regcache *regcache, int regnum)
>  {
>    struct gdbarch *gdbarch = regcache->arch ();
>    ptid_t ptid = regcache->ptid ();
> @@ -208,6 +216,13 @@ sparc_store_inferior_registers (struct regcache *regcache, int regnum)
>  	  ULONGEST sp;
>  
>  	  regcache_cooked_read_unsigned (regcache, SPARC_SP_REGNUM, &sp);
> +
> +	  /* Deep down, sparc_collect_rwindow writes memory, so needs the global
> +	     thread context to be set.  */
> +	  thread_info *thread = find_thread_ptid (proc_target, ptid);
> +	  scoped_restore_current_thread restore_thread;
> +	  switch_to_thread (thread);
> +
>  	  sparc_collect_rwindow (regcache, sp, regnum);
>  	}
>  
> diff --git a/gdb/sparc-nat.h b/gdb/sparc-nat.h
> index 196bf1f2307e..9d0c24f731f6 100644
> --- a/gdb/sparc-nat.h
> +++ b/gdb/sparc-nat.h
> @@ -41,8 +41,10 @@ extern int (*sparc_fpregset_supplies_p) (struct gdbarch *gdbarch, int);
>  extern int sparc32_gregset_supplies_p (struct gdbarch *gdbarch, int regnum);
>  extern int sparc32_fpregset_supplies_p (struct gdbarch *gdbarch, int regnum);
>  
> -extern void sparc_fetch_inferior_registers (struct regcache *, int);
> -extern void sparc_store_inferior_registers (struct regcache *, int);
> +extern void sparc_fetch_inferior_registers (process_stratum_target *proc_target,
> +					    regcache *, int);
> +extern void sparc_store_inferior_registers (process_stratum_target *proc_target,
> +					    regcache *, int);
>  
>  extern target_xfer_status sparc_xfer_wcookie (enum target_object object,
>  					      const char *annex,
> @@ -59,10 +61,10 @@ template<typename BaseTarget>
>  struct sparc_target : public BaseTarget
>  {
>    void fetch_registers (struct regcache *regcache, int regnum) override
> -  { sparc_fetch_inferior_registers (regcache, regnum); }
> +  { sparc_fetch_inferior_registers (this, regcache, regnum); }
>  
>    void store_registers (struct regcache *regcache, int regnum) override
> -  { sparc_store_inferior_registers (regcache, regnum); }
> +  { sparc_store_inferior_registers (this, regcache, regnum); }
>  
>    enum target_xfer_status xfer_partial (enum target_object object,
>  					const char *annex,
> diff --git a/gdb/sparc-obsd-tdep.c b/gdb/sparc-obsd-tdep.c
> index 6cd9227f8aed..e198e9702cdb 100644
> --- a/gdb/sparc-obsd-tdep.c
> +++ b/gdb/sparc-obsd-tdep.c
> @@ -25,6 +25,7 @@
>  #include "regcache.h"
>  #include "symtab.h"
>  #include "trad-frame.h"
> +#include "inferior.h"
>  
>  #include "obsd-tdep.h"
>  #include "sparc-tdep.h"
> @@ -158,6 +159,9 @@ sparc32obsd_supply_uthread (struct regcache *regcache,
>    CORE_ADDR fp, fp_addr = addr + SPARC32OBSD_UTHREAD_FP_OFFSET;
>    gdb_byte buf[4];
>  
> +  /* This function calls functions that depend on the global current thread.  */
> +  gdb_assert (regcache->ptid () == inferior_ptid);
> +
>    gdb_assert (regnum >= -1);
>  
>    fp = read_memory_unsigned_integer (fp_addr, 4, byte_order);
> @@ -203,6 +207,9 @@ sparc32obsd_collect_uthread(const struct regcache *regcache,
>    CORE_ADDR sp;
>    gdb_byte buf[4];
>  
> +  /* This function calls functions that depend on the global current thread.  */
> +  gdb_assert (regcache->ptid () == inferior_ptid);
> +
>    gdb_assert (regnum >= -1);
>  
>    if (regnum == SPARC_SP_REGNUM || regnum == -1)
> diff --git a/gdb/sparc-tdep.c b/gdb/sparc-tdep.c
> index 4f9c679b55cd..34f22879737a 100644
> --- a/gdb/sparc-tdep.c
> +++ b/gdb/sparc-tdep.c
> @@ -1948,6 +1948,9 @@ sparc_supply_rwindow (struct regcache *regcache, CORE_ADDR sp, int regnum)
>    gdb_byte buf[8];
>    int i;
>  
> +  /* This function calls functions that depend on the global current thread.  */
> +  gdb_assert (regcache->ptid () == inferior_ptid);
> +
>    if (sp & 1)
>      {
>        /* Registers are 64-bit.  */
> @@ -2022,6 +2025,9 @@ sparc_collect_rwindow (const struct regcache *regcache,
>    gdb_byte buf[8];
>    int i;
>  
> +  /* This function calls functions that depend on the global current thread.  */
> +  gdb_assert (regcache->ptid () == inferior_ptid);
> +
>    if (sp & 1)
>      {
>        /* Registers are 64-bit.  */
> diff --git a/gdb/sparc64-linux-nat.c b/gdb/sparc64-linux-nat.c
> index ad2cc35bd70f..b741d0216c32 100644
> --- a/gdb/sparc64-linux-nat.c
> +++ b/gdb/sparc64-linux-nat.c
> @@ -35,10 +35,10 @@ class sparc64_linux_nat_target final : public linux_nat_target
>  public:
>    /* Add our register access methods.  */
>    void fetch_registers (struct regcache *regcache, int regnum) override
> -  { sparc_fetch_inferior_registers (regcache, regnum); }
> +  { sparc_fetch_inferior_registers (this, regcache, regnum); }
>  
>    void store_registers (struct regcache *regcache, int regnum) override
> -  { sparc_store_inferior_registers (regcache, regnum); }
> +  { sparc_store_inferior_registers (this, regcache, regnum); }
>  
>    /* Override linux_nat_target low methods.  */
>  
> diff --git a/gdb/sparc64-obsd-tdep.c b/gdb/sparc64-obsd-tdep.c
> index 9e678d268ad7..cf3138a7ab14 100644
> --- a/gdb/sparc64-obsd-tdep.c
> +++ b/gdb/sparc64-obsd-tdep.c
> @@ -27,6 +27,7 @@
>  #include "symtab.h"
>  #include "objfiles.h"
>  #include "trad-frame.h"
> +#include "inferior.h"
>  
>  #include "obsd-tdep.h"
>  #include "sparc64-tdep.h"
> @@ -328,6 +329,9 @@ sparc64obsd_supply_uthread (struct regcache *regcache,
>    CORE_ADDR fp, fp_addr = addr + SPARC64OBSD_UTHREAD_FP_OFFSET;
>    gdb_byte buf[8];
>  
> +  /* This function calls functions that depend on the global current thread.  */
> +  gdb_assert (regcache->ptid () == inferior_ptid);
> +
>    gdb_assert (regnum >= -1);
>  
>    fp = read_memory_unsigned_integer (fp_addr, 8, byte_order);
> @@ -373,6 +377,9 @@ sparc64obsd_collect_uthread(const struct regcache *regcache,
>    CORE_ADDR sp;
>    gdb_byte buf[8];
>  
> +  /* This function calls functions that depend on the global current thread.  */
> +  gdb_assert (regcache->ptid () == inferior_ptid);
> +
>    gdb_assert (regnum >= -1);
>  
>    if (regnum == SPARC_SP_REGNUM || regnum == -1)
> -- 
> 2.30.1
> 

Re: [PATCH gdb-10-branch candidate] gdb: set current thread in sparc_{fetch,collect}_inferior_registers (PR gdb/27147)

[Simon Marchi]

> LGTM.
> 
> Thanks,
> Andrew

Thanks, pushed to master and gdb-10-branch.

Simon