Re: [PATCHv3 6/6] gdb: native target invalid architecture detection

[Pedro Alves <pedro@palves.net>]

On 2022-06-27 17:27, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
>> On 2022-06-13 17:15, Andrew Burgess via Gdb-patches wrote:
>> I have another question.  I'm confused on how if e.g., on x86-64, you
>> load a RISC-V binary into GDB, and the try to run it, we don't end up
>> with architecture of the target description instead.
> 
> The problem is that we end up trying to read the registers before we
> read the target description.  Which I suspect you will say sounds wrong,
> and I agree.
> 
> I removed the error() calls from this patch, and reran GDB, here's the
> stack at the point of failure:
> 

...

>   #11 0x0000000000bf43c4 in regcache_read_pc (regcache=0x30f5f10) at ../../src/gdb/regcache.c:1325
>   #12 0x00000000009c2e19 in save_stop_reason (lp=0x3a8ff80) at ../../src/gdb/linux-nat.c:2545
>   #13 0x00000000009c40f0 in linux_nat_filter_event (lwpid=3990223, status=1407) at ../../src/gdb/linux-nat.c:3003
>   #14 0x00000000009c471a in linux_nat_wait_1 (ptid=..., ourstatus=0x7fffffffa4f0, target_options=...)
>       at ../../src/gdb/linux-nat.c:3178
>   #15 0x00000000009c535a in linux_nat_target::wait (this=0x2d27140 <the_amd64_linux_nat_target>, ptid=..., 
>       ourstatus=0x7fffffffa4f0, target_options=...) at ../../src/gdb/linux-nat.c:3416
>   #16 0x0000000000dc5f3b in target_wait (ptid=..., status=0x7fffffffa4f0, options=...)
>       at ../../src/gdb/target.c:2601
>   #17 0x0000000000abd3c4 in startup_inferior (proc_target=0x2d27140 <the_amd64_linux_nat_target>, pid=3990223, 
>       ntraps=1, last_waitstatus=0x0, last_ptid=0x0) at ../../src/gdb/nat/fork-inferior.c:482

Alright, we're inside startup_inferior.

> 
> The initial architecture is riscv:rv32, and it is this architecture that
> is used for handling the initial stop.
> 
> So my first thought was, pretty much as you suggest, that we should
> fetch the target description earlier, then we'll not run into this
> problem at all.
> 

I don't think we should fetch the tdesc earlier, I think that would be incorrect.  Instead,
we should avoid reading registers while going through the shell.  The shell's registers have
nothing to do with the final program's registers.  The shell may even be of a different
architecture from the final inferior's architecture.  Like, if your shell is a 64-bit
bash, and then the inferior you're going to debug, which the shell execs, is a 32-bit program.

> My problem was figuring out the right place to add such a call.
> 
> We know it has to be before we enter regcache_read_pc, as at this point
> the regcache is already created, and has the wrong architecture.
> 
> And it can't really be earlier than linux_nat_filter_event I think, as
> before this point we don't know that the target has stopped.  We can't
> probe the target for features (I would assume) if the target is not
> currently stopped.
> 
> This seems to leave either save_stop_reason, or in the early stages of
> get_thread_regcache maybe.
> 
> The problem with save_stop_reason is that it's Linux specific, so any
> non-Linux targets will need to add their own code to catch this
> situation.

I think we should change save_stop_reason.  But, change it to avoid reading
the PC instead.

I don't think that the fact that other targets need to handle this too
is an issue.

>> It kind of sounds like if GDB detected the incompatibility when we first fetch the
>> target description, similarly to how we detect it in the cases above, and GDB picked
>> the target arch or errored out after detecting the incompatibility, then we wouldn't
>> need a patch like yours?
> 
> I agree.  One other aspect is, do we have any native targets that don't
> support reading a target description?  Hopefully not.

If we do, we have an action plan -- add such support.  I think a tdesc can be just an
<architecture> element, for instance.

> 
>>                          How come the mismatch isn't detected then (when we first
>> retrieve the target description) today?
> 
> Hopefully the above makes that clear, but the TLDR answer is: we attempt
> to read the registers using the current architecture before we fetch the
> target description.
> 
> I suspected that, in part, this patch would be a bit of a straw-man, I'd
> appreciate any insight you have for how we might reorder things so that
> the target description can be read before we try to read the registers.
> 

So I gave the patch below a try, and it seems to works well -- readcache_read_pc
is no longer called before target_find_description, and I saw no testsuite
regressions (this is without your series, just pristine master).

Now, this patch as is makes sense for linux-nat.c, but then I went to do the
same thing for gdbserver, which has an equivalent save_stop_reason in linux-low.cc,
and there this approach won't work as is for the simple fact that lwp->stop_pc
is used in a lot more places.

So to avoid gdb vs gdbserver divergence, it may be better to not take this patch
as is, but instead tweak save_stop_reason to avoid reading the stop pc while
the inferior is going through the shell, based on some per-inferior flag or some
such.  Maybe we can repurpose progspace->executing_startup for this?  That
flag used to be used by Cell (which is gone), but seems unused nowadays.  There's
also other similar flags in inferior itself, like needs_setup, and
in_initial_library_scan.  So we'd add a flag like one of those, set it while the
inferior is going through the startup_inferior dance, and make save_stop_reason avoid
reading the PC while that flag is set.

Note the patch below applies on top of:

  [PATCH] gdb+gdbserver/Linux: Remove USE_SIGTRAP_SIGINFO fallback
  https://sourceware.org/pipermail/gdb-patches/2022-June/190372.html

Attaching will need some other fix, because in that case, the procedure is that the core
requests an attach, and then the target reports an initial stop, and that stop
makes infrun read the thread's PC, before setup_inferior is called.  It may be
we need to call setup_inferior earlier.  Or maybe we can just add a target_find_description
call in linux_nat_target::attach, like remote.c does:

 void
 extended_remote_target::attach (const char *args, int from_tty)
 {
 ...
   /* Next, if the target can specify a description, read it.  We do
      this before anything involving memory or registers.  */
   target_find_description ();

I haven't looked at that in much detail, and I have to leave for tonight.

From 303d216239ba45c8930bd4291b9f49aae105ada7 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Mon, 27 Jun 2022 20:41:50 +0100
Subject: [PATCH] gdb/Linux: avoid reading registers while going through shell

For every stop, linux-nat.c saves the stopped thread's PC, in
lwp->stop_pc.  This is done in save_stop_reason.  However, while we're
going through the shell after "run", in startup_inferior, we shouldn't
be reading registers, as the shell's architecture may not even be the
same as the final inferior's.

Since lwp->stop_pc is only needed when the thread has stopped for a
breakpoint, and since when going through the shell, no breakpoint is
going to hit, we can simple teach save_stop_reason to only record the
stop pc when the thread stopped for a breakpoint.

Change-Id: If0f01831514d3a74d17efd102875de7d2c6401ad
---
 gdb/linux-nat.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 008791c12dc..27437da7c94 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2514,24 +2514,20 @@ select_event_lwp_callback (struct lwp_info *lp, int *selector)
 static void
 save_stop_reason (struct lwp_info *lp)
 {
-  struct regcache *regcache;
-  struct gdbarch *gdbarch;
-  CORE_ADDR pc;
-  CORE_ADDR sw_bp_pc;
-  siginfo_t siginfo;
-
   gdb_assert (lp->stop_reason == TARGET_STOPPED_BY_NO_REASON);
   gdb_assert (lp->status != 0);
 
   if (!linux_target->low_status_is_event (lp->status))
     return;
 
-  regcache = get_thread_regcache (linux_target, lp->ptid);
-  gdbarch = regcache->arch ();
-
-  pc = regcache_read_pc (regcache);
-  sw_bp_pc = pc - gdbarch_decr_pc_after_break (gdbarch);
+  /* Note we avoid reading this until we know we have a sw/hw
+     breakpoint stop, which are the cases we actually need it, see
+     status_callback.  This is to avoid reading registers while we're
+     going through the shell, before we're able to determine the
+     debuggee's target description.  */
+  lp->stop_pc = 0;
 
+  siginfo_t siginfo;
   if (linux_nat_get_siginfo (lp->ptid, &siginfo))
     {
       if (siginfo.si_signo == SIGTRAP)
@@ -2580,25 +2576,31 @@ save_stop_reason (struct lwp_info *lp)
       linux_nat_debug_printf ("%s stopped by software breakpoint",
 			      lp->ptid.to_string ().c_str ());
 
-      /* Back up the PC if necessary.  */
-      if (pc != sw_bp_pc)
-	regcache_write_pc (regcache, sw_bp_pc);
+      struct regcache *regcache = get_thread_regcache (linux_target, lp->ptid);
+
+      lp->stop_pc = regcache_read_pc (regcache);
 
-      /* Update this so we record the correct stop PC below.  */
-      pc = sw_bp_pc;
+      /* Back up the PC if necessary.  */
+      CORE_ADDR decr_pc = gdbarch_decr_pc_after_break (regcache->arch ());
+      if (decr_pc != 0)
+	{
+	  lp->stop_pc -= decr_pc;
+	  regcache_write_pc (regcache, lp->stop_pc);
+	}
     }
   else if (lp->stop_reason == TARGET_STOPPED_BY_HW_BREAKPOINT)
     {
       linux_nat_debug_printf ("%s stopped by hardware breakpoint",
 			      lp->ptid.to_string ().c_str ());
+
+      struct regcache *regcache = get_thread_regcache (linux_target, lp->ptid);
+      lp->stop_pc = regcache_read_pc (regcache);
     }
   else if (lp->stop_reason == TARGET_STOPPED_BY_WATCHPOINT)
     {
       linux_nat_debug_printf ("%s stopped by hardware watchpoint",
 			      lp->ptid.to_string ().c_str ());
     }
-
-  lp->stop_pc = pc;
 }
 
 

base-commit: f18acc9c4e5d18f4783f3a7d59e3ec95d7af0199
prerequisite-patch-id: 9656309f5c298727d3cf993bed0bb8e00c9e9d56
-- 
2.36.0