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

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> On 2022-06-13 17:15, Andrew Burgess via Gdb-patches wrote:
>
>> This leaves just one question, what about native targets that support
>> multiple architectures?
>> 
>
> 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:

  #0  internal_error (file=0x16bcfc8 "../../src/gdb/../gdbsupport/gdb-checked-static-cast.h", line=60, 
      fmt=0x16bcf80 "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:54
  #1  0x00000000004c4f4c in gdb::checked_static_cast<i386_gdbarch_tdep*, gdbarch_tdep> (v=0x337b910)
      at ../../src/gdb/../gdbsupport/gdb-checked-static-cast.h:60
  #2  0x00000000004d09c2 in gdbarch_tdep<i386_gdbarch_tdep> (gdbarch=0x3a4c630) at ../../src/gdb/gdbarch.h:167
  #3  0x00000000004d059e in amd64_supply_fxsave (regcache=0x30f5f10, regnum=-1, fxsave=0x7fffffff9070)
      at ../../src/gdb/amd64-tdep.c:3387
  #4  0x00000000004c5958 in amd64_linux_nat_target::fetch_registers (this=0x2d27140 <the_amd64_linux_nat_target>, 
      regcache=0x30f5f10, regnum=32) at ../../src/gdb/amd64-linux-nat.c:260
  #5  0x0000000000dd74bd in target_fetch_registers (regcache=0x30f5f10, regno=32) at ../../src/gdb/target.c:3948
  #6  0x0000000000bf2540 in regcache::raw_update (this=0x30f5f10, regnum=32) at ../../src/gdb/regcache.c:587
  #7  0x0000000000bf25e5 in readable_regcache::raw_read (this=0x30f5f10, regnum=32, 
      buf=0x7fffffff9c60 "\200\234\377\377\377\177") at ../../src/gdb/regcache.c:601
  #8  0x0000000000bf291d in readable_regcache::cooked_read (this=0x30f5f10, regnum=32, 
      buf=0x7fffffff9c60 "\200\234\377\377\377\177") at ../../src/gdb/regcache.c:690
  #9  0x0000000000bf9d6a in readable_regcache::cooked_read<unsigned long, void> (this=0x30f5f10, regnum=32, 
      val=0x7fffffff9d28) at ../../src/gdb/regcache.c:775
  #10 0x0000000000bf2da9 in regcache_cooked_read_unsigned (regcache=0x30f5f10, regnum=32, val=0x7fffffff9d28)
      at ../../src/gdb/regcache.c:789
  #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
  #18 0x00000000008a0165 in gdb_startup_inferior (pid=3990223, num_traps=1) at ../../src/gdb/fork-child.c:129
  #19 0x0000000000936f6e in inf_ptrace_target::create_inferior (this=0x2d27140 <the_amd64_linux_nat_target>, 
      exec_file=0x337c1b0 "/tmp/hello.rv32imc.x", allargs=..., env=0x2dba940, from_tty=1)
      at ../../src/gdb/inf-ptrace.c:105
  #20 0x00000000009bef5b in linux_nat_target::create_inferior (this=0x2d27140 <the_amd64_linux_nat_target>, 
      exec_file=0x337c1b0 "/tmp/hello.rv32imc.x", allargs=..., env=0x2dba940, from_tty=1)
      at ../../src/gdb/linux-nat.c:977
  #21 0x00000000009424ee in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../src/gdb/infcmd.c:461
  #22 0x0000000000942784 in run_command (args=0x0, from_tty=1) at ../../src/gdb/infcmd.c:519

In contrast, the call to target_find_description is done (indirectly)
from later in run_command_1, like this:

  #0  target_find_description () at ../../src.dev-4/gdb/target-descriptions.c:521
  #1  0x0000000000941e7c in post_create_inferior (from_tty=0) at ../../src.dev-4/gdb/infcmd.c:253
  #2  0x00000000009425ea in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../src.dev-4/gdb/infcmd.c:494
  #3  0x0000000000942784 in run_command (args=0x0, from_tty=1) at ../../src.dev-4/gdb/infcmd.c:519

So, after I start GDB like:

  $ arch
  x86_64
  $ gdb -q /tmp/hello.rv32imc.x
  Reading symbols from /tmp/hello.rv32imc.x...
  (gdb) show arch
  The target architecture is set to "auto" (currently "riscv:rv32").
  (gdb) run
  ...

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.

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.

Inside the get_thread_regcache code is possible I guess, but something
about it seemed off, I guess it was the idea of calling
get_thread_regcache and having the thread's architecture change under
your feet - that seemed pretty unexpected to me.

>                                                       If the target
> reports a target description, and the resulting architecture is not
> compatible with the executable, then I think it would make sense to
> use the architecture of the target?

I agree.  For a native target once the target actually tries to start
the executable I think we'd expect the target itself to report an error
relating to the wrong architecture.

>
> If I start a 64-bit program on x86-64, like:
>
>  $ gdb ~/gdb/tests/main64
>  GNU gdb (GDB) 13.0.50.20220624-git
>  ...
>  (gdb) start
>  Temporary breakpoint 1 at 0x400492: file main.c, line 11.
>  Starting program: /home/pedro/gdb/tests/main64
>
>  Temporary breakpoint 1, main (argc=1, argv=0x7fffffffdc48) at main.c:11
>  11        return 0;
>
> and then try to load the executable of an incompatible arch, like:
>
>  (gdb) file ~/gdb/tests/main32
>  A program is being debugged already.
>  Are you sure you want to change the file? (y or n) y
>  warning: Selected architecture i386 is not compatible with reported target architecture i386:x86-64
>  Architecture of file not recognized.
>
> then GDB doesn't switch the architecture to the file's architecture, it is still x86-64:
>
>  (gdb) show architecture 
>  The target architecture is set to "auto" (currently "i386:x86-64").
>
> and it doesn't let me manually select an incompatible architecture either:
>
>  (gdb) set architecture riscv
>  warning: Selected architecture riscv is not compatible with reported target architecture i386:x86-64
>  Architecture `riscv' not recognized.

[ Unrelated: We should probably remove that 'Architecture `riscv' not
  recognized.' line, as that seems wrong here.  The architecture is
  recognized, it's just not compatible.]

>  The target architecture is set to "auto" (currently "i386:x86-64").
>  (gdb) 
>
> 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.

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

Thanks,
Andrew



>
>> +
>>  protected:
>>    /* Unpush the target if it wasn't explicitly open with "target native"
>>       and there are no live inferiors left.  Note: if calling this as a
>> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
>> index e909d4d4c81..b8acf858b3a 100644
>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -413,6 +413,10 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
>>    if (non_stop && !run_target->supports_non_stop ())
>>      error (_("The target does not support running in non-stop mode."));
>>  
>> +  if (!run_target->supports_architecture_p (get_current_arch ()))
>> +    error (_("The target does not support architecture \"%s\"."),
>> +	   gdbarch_bfd_arch_info (get_current_arch ())->printable_name);
>> +
>>    /* Done.  Can now set breakpoints, change inferior args, etc.  */
>>  
>>    /* Insert temporary breakpoint in main function if requested.  */
>> @@ -2590,6 +2594,10 @@ attach_command (const char *args, int from_tty)
>>    if (non_stop && !attach_target->supports_non_stop ())
>>      error (_("Cannot attach to this target in non-stop mode"));
>>  
>> +  if (!attach_target->supports_architecture_p (get_current_arch ()))
>> +    error (_("The target does not support architecture \"%s\"."),
>> +	   gdbarch_bfd_arch_info (get_current_arch ())->printable_name);
>> +
>
> Since the error is the same in both cases, I'd suggest putting it in
> a shared routine, like:
>
>  static void
>  check_target_supports_current_arch (process_stratum_target *target)
>  {
>    if (!target->supports_architecture_p (get_current_arch ()))
>      error (_("The target does not support architecture \"%s\"."),
>  	   gdbarch_bfd_arch_info (get_current_arch ())->printable_name);
>  }