[PATCH 5/5] gdb: native target invalid architecture detection

[Andrew Burgess <aburgess@redhat.com>]

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 make use of this from a GDB session 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.

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.
---
 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/target-delegates.c | 28 ++++++++++++++++++++++++++++
 gdb/target.h           | 13 ++++++++++++-
 7 files changed, 82 insertions(+), 1 deletion(-)

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