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

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> 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.

Thanks, this looks good.  Maybe you should go ahead and merge this, and
the attach problem can be solved separately.

Thanks,
Andrew


>
> 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