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

[Luis Machado <luis.machado@arm.com>]

On 6/10/22 14:08, Andrew Burgess via Gdb-patches wrote:
> If GDB is asked to start a new inferior, or attach to an existing
> process, using a binary file for an architecture that does not match
> the current native target, then, currently, GDB will assert.  Here's
> an example session using current HEAD of master with GDB built for an
> x86-64 GNU/Linux native target, the binary being used is a RISC-V ELF:
> 
>    $ ./gdb/gdb -q --data-directory ./gdb/data-directory/
>    (gdb) file /tmp/hello.rv32imc.x
>    Reading symbols from /tmp/hello.rv32imc.x...
>    (gdb) start
>    Temporary breakpoint 1 at 0x101b2: file hello.rv32.c, line 23.
>    Starting program: /tmp/hello.rv32imc.x
>    ../../src/gdb/gdbarch.h:166: internal-error: gdbarch_tdep: Assertion `dynamic_cast<TDepType *> (tdep) != nullptr' failed.
>    A problem internal to GDB has been detected,
>    further debugging may prove unreliable.
> 
> The same error is encountered if, instead of starting a new inferior,
> the user tries to attach to an x86-64 process with a RISC-V binary set
> as the current executable.
> 
> These errors are not specific to the x86-64/RISC-V pairing I'm using
> here, any attempt to use a binary for one architecture with a native
> target of a different architecture will result in a similar error.
> 
> Clearly, attempting to use this cross-architecture combination is a
> user error, but I think GDB should do better than an assert; ideally a
> nice error should be printed.
> 
> The problem we run into is that, when the user starts a new inferior,
> or attaches to an inferior, the inferior stops.  At this point GDB
> attempts to handle the stop, and this involves reading registers from
> the inferior.
> 
> These register reads end up being done through the native target, so
> in the example above, we end up in the amd64_supply_fxsave function.
> However, these functions need a gdbarch.  The gdbarch is fetched from
> the register set, which was constructed using the gdbarch from the
> binary currently in use.  And so we end up in amd64_supply_fxsave
> using a RISC-V gdbarch.
> 
> When we call:
> 
>    i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
> 
> this will assert as the gdbarch_tdep data within the RISC-V gdbarch is
> of the type riscv_gdbarch_tdep not i386_gdbarch_tdep.
> 
> The solution I propose in this commit is to add a new target_ops
> method supports_architecture_p.  This method will return true if a
> target can safely be used with a specific architecture, otherwise, the
> method returns false.
> 
> I imagine that a result of true from this method doesn't guarantee
> that GDB can start an inferior of a given architecture, it just means
> that GDB will not crash if such an attempt is made.  A result of false
> is a hard stop; attempting to use this target with this architecture
> is not supported, and may cause GDB to crash.
> 
> This distinction is important I think for things like remote targets,
> and possibly simulator targets.  We might imagine that GDB can ask a
> remote (or simulator) to start with a particular executable, and the
> target might still refuse for some reason.  But my thinking is that
> these refusals should be well handled (i.e. GDB should give a user
> friendly error), rather than crashing, as is the case with the native
> targets.
> 
> For example, if I start gdbserver on an x86-64 machine like this:
> 
>    gdbserver --multi :54321
> 
> Then use GDB to try and load a RISC-V binary, like this:
> 
>    $ ./gdb/gdb -q --data-directory ./gdb/data-directory/
>    (gdb) file /tmp/hello.rv32imc.x
>    Reading symbols from /tmp/hello.rv32imc.x...
>    (gdb) target extended-remote :54321
>    Remote debugging using :54321
>    (gdb) run
>    Starting program: /tmp/hello.rv32imc.x
>    Running the default executable on the remote target failed; try "set remote exec-file"?
>    (gdb)
> 
> Though the error is not very helpful in diagnosing the problem, we can
> see that GDB has not crashed, but has given the user an error.
> 
> And so, the supports_architecture_p method is created to return true
> by default, then I override this in inf_child_target, where I compare
> the architecture in question with the default_bfd_arch.
> 
> Finally, I've added calls to supports_architecture_p for the
> run (which covers run, start, starti) and attach commands.
> 
> This leaves just one question, what about native targets that support
> multiple architectures?
> 
> These targets can be split into two groups.  First, we have targets
> like x86-64, which also supports i386 binaries.  This case is easy to
> handle, as far as BFD is concerned there is only one architecture,
> bfd_arch_i386, and we then use machine types to split this
> architecture into x86-64 and i386 (and others).  As the new
> supports_architecture_p function only checks the bfd architecture,
> then there is nothing additional needed for this case.
> 
> The second group of multi-architecture targets requires more work.
> The only targets that I'm aware of that fall into this group are the
> rs6000-aix-nat.c, ppc-*-nat.c targets, and the aarch64-linux-nat.c
> target.
> 
> The first group (rs600/ppc) support bfd_arch_rs6000 and
> bfd_arch_powerpc, while the second (aarch64) supports bfd_arch_arm and
> bfd_arch_aarch64.
> 
> To deal with these targets I have overridden the
> supports_architecture_p function in each of the separate target files,
> these overrides check both of the supported architectures.
> 
> One final note, in the rs6000/ppc case, the FreeBSD target supports
> both architectures, and so we override supports_architecture_p.  In
> contrast, the aarch64_fbsd_nat_target target does not (yet) support
> bfd_arch_arm, and so there is no supports_architecture_p here.  This
> can always be added later if/when support is added.
> 
> You will notice a lack of tests for this change.  I'm not sure of a
> good way that I can build a binary for a different architecture as
> part of a test, but if anyone has any ideas then I'll be happy to add
> a test here.  The gdb.base/multi-arch.exp test exists, which for
> AArch64 will test compiling and running something as both AArch64 and
> ARM, but this doesn't cover the error case, just that the overridden
> supports_architecture_p works in that case.
> ---
>   gdb/aarch64-linux-nat.c |  8 ++++++++
>   gdb/arch-utils.c        |  8 ++++++++
>   gdb/arch-utils.h        |  5 +++++
>   gdb/inf-child.c         | 19 +++++++++++++++++++
>   gdb/inf-child.h         |  2 ++
>   gdb/infcmd.c            |  8 ++++++++
>   gdb/ppc-fbsd-nat.c      |  8 ++++++++
>   gdb/ppc-linux-nat.c     |  8 ++++++++
>   gdb/ppc-obsd-nat.c      |  8 ++++++++
>   gdb/rs6000-aix-nat.c    |  8 ++++++++
>   gdb/target-delegates.c  | 28 ++++++++++++++++++++++++++++
>   gdb/target.h            | 13 ++++++++++++-
>   12 files changed, 122 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index a457fcd48ad..6114c4c6638 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -108,6 +108,14 @@ class aarch64_linux_nat_target final
>     /* Write allocation tags to memory via PTRACE.  */
>     bool store_memtags (CORE_ADDR address, size_t len,
>   		      const gdb::byte_vector &tags, int type) override;
> +
> +  /* This target supports two architectures, check for them both here.  */
> +
> +  bool supports_architecture_p (struct gdbarch *gdbarch) override
> +  {
> +    bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch;
> +    return (the_arch == bfd_arch_aarch64 || the_arch == bfd_arch_arm);
> +  }
>   };
>   
>   static aarch64_linux_nat_target the_aarch64_linux_nat_target;
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 360b8d694be..efba17e1d5d 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1570,6 +1570,14 @@ target_gdbarch (void)
>     return current_inferior ()->gdbarch;
>   }
>   
> +/* See arch-utils.h.  */
> +
> +bool
> +gdbarch_matches_default_arch (struct gdbarch *gdbarch)
> +{
> +  return gdbarch_bfd_arch_info (gdbarch)->arch == default_bfd_arch->arch;
> +}
> +
>   void _initialize_gdbarch_utils ();
>   void
>   _initialize_gdbarch_utils ()
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index f850e5fd6e7..5d61be36c26 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -300,4 +300,9 @@ extern void default_read_core_file_mappings
>      struct bfd *cbfd,
>      read_core_file_mappings_pre_loop_ftype pre_loop_cb,
>      read_core_file_mappings_loop_ftype loop_cb);
> +
> +/* Return true if the bfd architecture of GDBARCH matches the default bfd
> +   architecture.  */
> +
> +extern bool gdbarch_matches_default_arch (struct gdbarch *gdbarch);
>   #endif /* ARCH_UTILS_H */
> diff --git a/gdb/inf-child.c b/gdb/inf-child.c
> index 56ebd2a5549..0c47a367c68 100644
> --- a/gdb/inf-child.c
> +++ b/gdb/inf-child.c
> @@ -30,6 +30,8 @@
>   #include "inferior.h"
>   #include <sys/stat.h>
>   #include "inf-child.h"
> +#include "gdbarch.h"
> +#include "arch-utils.h"
>   #include "gdbsupport/fileio.h"
>   #include "gdbsupport/agent.h"
>   #include "gdbsupport/gdb_wait.h"
> @@ -412,6 +414,23 @@ inf_child_target::follow_exec (inferior *follow_inf, ptid_t ptid,
>       }
>   }
>   
> +/* The inf_child_target represents the native target built into GDB, of
> +   which there is only ever one.  If we attempt to start a native inferior
> +   using a binary for an architecture not matching the native target then,
> +   when we handle the first stop, we will end up trying to read registers
> +   using the gdbarch functions from the native target, but passing in a
> +   gdbarch object based on the architecture of the binary file.  This will
> +   result in errors.
> +
> +   This check prevents this so long as everywhere user command that might
> +   cause a new inferior to be created calls this function.  */
> +
> +bool
> +inf_child_target::supports_architecture_p (struct gdbarch *gdbarch)
> +{
> +  return gdbarch_matches_default_arch (gdbarch);
> +}
> +
>   /* See inf-child.h.  */
>   
>   void
> diff --git a/gdb/inf-child.h b/gdb/inf-child.h
> index ae5ace46f30..01a8164b0c2 100644
> --- a/gdb/inf-child.h
> +++ b/gdb/inf-child.h
> @@ -92,6 +92,8 @@ class inf_child_target
>   
>     bool can_use_agent () override;
>   
> +  bool supports_architecture_p (struct gdbarch *gdbarch) override;
> +
>   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);
> +
>     attach_target->attach (args, from_tty);
>     /* to_attach should push the target, so after this point we
>        shouldn't refer to attach_target again.  */
> diff --git a/gdb/ppc-fbsd-nat.c b/gdb/ppc-fbsd-nat.c
> index d0a5778e2d3..f7c36c33958 100644
> --- a/gdb/ppc-fbsd-nat.c
> +++ b/gdb/ppc-fbsd-nat.c
> @@ -41,6 +41,14 @@ struct ppc_fbsd_nat_target final : public fbsd_nat_target
>   {
>     void fetch_registers (struct regcache *, int) override;
>     void store_registers (struct regcache *, int) override;
> +
> +  /* This target supports two architectures, check for them both here.  */
> +
> +  bool supports_architecture_p (struct gdbarch *gdbarch) override
> +  {
> +    bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch;
> +    return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc);
> +  }
>   };
>   
>   static ppc_fbsd_nat_target the_ppc_fbsd_nat_target;
> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
> index de4158c411a..e302b15925a 100644
> --- a/gdb/ppc-linux-nat.c
> +++ b/gdb/ppc-linux-nat.c
> @@ -551,6 +551,14 @@ struct ppc_linux_nat_target final : public linux_nat_target
>   
>     void low_prepare_to_resume (struct lwp_info *) override;
>   
> +  /* This target supports two architectures, check for them both here.  */
> +
> +  bool supports_architecture_p (struct gdbarch *gdbarch) override
> +  {
> +    bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch;
> +    return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc);
> +  }
> +
>   private:
>   
>     void copy_thread_dreg_state (const ptid_t &parent_ptid,
> diff --git a/gdb/ppc-obsd-nat.c b/gdb/ppc-obsd-nat.c
> index e480f19dc73..2fdccc3085d 100644
> --- a/gdb/ppc-obsd-nat.c
> +++ b/gdb/ppc-obsd-nat.c
> @@ -39,6 +39,14 @@ struct ppc_obsd_nat_target final : public obsd_nat_target
>   {
>     void fetch_registers (struct regcache *, int) override;
>     void store_registers (struct regcache *, int) override;
> +
> +  /* This target supports two architectures, check for them both here.  */
> +
> +  bool supports_architecture_p (struct gdbarch *gdbarch) override
> +  {
> +    bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch;
> +    return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc);
> +  }
>   };
>   
>   static ppc_obsd_nat_target the_ppc_obsd_nat_target;
> diff --git a/gdb/rs6000-aix-nat.c b/gdb/rs6000-aix-nat.c
> index 8697d27b4ed..603caa8fb1e 100644
> --- a/gdb/rs6000-aix-nat.c
> +++ b/gdb/rs6000-aix-nat.c
> @@ -91,6 +91,14 @@ class rs6000_nat_target final : public inf_ptrace_target
>   
>     ptid_t wait (ptid_t, struct target_waitstatus *, target_wait_flags) override;
>   
> +  /* This target supports two architectures, check for them both here.  */
> +
> +  bool supports_architecture_p (struct gdbarch *gdbarch) override
> +  {
> +    bfd_architecture the_arch = gdbarch_bfd_arch_info (gdbarch)->arch;
> +    return (the_arch == bfd_arch_rs6000 || the_arch == bfd_arch_powerpc);
> +  }
> +
>   protected:
>   
>     void post_startup_inferior (ptid_t ptid) override
> diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
> index 8a9986454dd..cb7bb1f2062 100644
> --- a/gdb/target-delegates.c
> +++ b/gdb/target-delegates.c
> @@ -196,6 +196,7 @@ struct dummy_target : public target_ops
>     bool supports_memory_tagging () override;
>     bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>     bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
> +  bool supports_architecture_p (struct gdbarch *arg0) override;
>   };
>   
>   struct debug_target : public target_ops
> @@ -370,6 +371,7 @@ struct debug_target : public target_ops
>     bool supports_memory_tagging () override;
>     bool fetch_memtags (CORE_ADDR arg0, size_t arg1, gdb::byte_vector &arg2, int arg3) override;
>     bool store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector &arg2, int arg3) override;
> +  bool supports_architecture_p (struct gdbarch *arg0) override;
>   };
>   
>   void
> @@ -4536,3 +4538,29 @@ debug_target::store_memtags (CORE_ADDR arg0, size_t arg1, const gdb::byte_vector
>     return result;
>   }
>   
> +bool
> +target_ops::supports_architecture_p (struct gdbarch *arg0)
> +{
> +  return this->beneath ()->supports_architecture_p (arg0);
> +}
> +
> +bool
> +dummy_target::supports_architecture_p (struct gdbarch *arg0)
> +{
> +  return true;
> +}
> +
> +bool
> +debug_target::supports_architecture_p (struct gdbarch *arg0)
> +{
> +  bool result;
> +  gdb_printf (gdb_stdlog, "-> %s->supports_architecture_p (...)\n", this->beneath ()->shortname ());
> +  result = this->beneath ()->supports_architecture_p (arg0);
> +  gdb_printf (gdb_stdlog, "<- %s->supports_architecture_p (", this->beneath ()->shortname ());
> +  target_debug_print_struct_gdbarch_p (arg0);
> +  gdb_puts (") = ", gdb_stdlog);
> +  target_debug_print_bool (result);
> +  gdb_puts ("\n", gdb_stdlog);
> +  return result;
> +}
> +
> diff --git a/gdb/target.h b/gdb/target.h
> index 18559feef89..5f3ae77f559 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -1320,7 +1320,18 @@ struct target_ops
>       virtual bool store_memtags (CORE_ADDR address, size_t len,
>   				const gdb::byte_vector &tags, int type)
>         TARGET_DEFAULT_NORETURN (tcomplain ());
> -  };
> +
> +    /* Return false if the target does not support GDBARCH, otherwise,
> +       return true.
> +
> +       The default reply of true does not guarantee that debugging an
> +       inferior with architecture GDBARCH will succeed, other errors might
> +       be thrown later on, but a return value of false is a clear
> +       indication that we should not proceed attempting to use this
> +       architecture with this target.  */
> +    virtual bool supports_architecture_p (struct gdbarch *gdbarch)
> +      TARGET_DEFAULT_RETURN (true);
> +};
>   
>   /* Deleter for std::unique_ptr.  See comments in
>      target_ops::~target_ops and target_ops::close about heap-allocated

LGTM from aarch64's side.