[PATCH 4/5] gdb: ensure the cast in gdbarch_tdep is valid

[Andrew Burgess]

This commit builds on the previous commit and modifies the
gdbarch_tdep function to ensure that the cast being performed is
valid.

To do this I make use of dynamic_cast to ensure that the generic
gdbarch_tdep pointer that we have is of the correct type.

The only problem with this approach is that, in order to use
dynamic_cast, we need RTTI information, which requires the class to
have a vtable, which currently, is not something the various tdep
classes have.

And so, in this commit, I add a virtual destructor to the gdbarch_tdep
class.

With this change I can now add an assert in the gdbarch_tdep function.

Obviously, this change comes at a cost, creation of the tdep classes
is now slightly more expensive (due to vtable initialisation),
however, this only happens when a new gdbarch is created, which is not
that frequent, so I don't see that as a huge concern.

Then, there is an increased cost each time the tdep is accessed.  This
is much more frequent, but I don't believe the cost is excessive (a
vtable pointer comparison), at least, no worse than many of our other
asserts.

If we consider the motivating example that was discussed in the
previous commit; build GDB for all targets on an x86-64 GNU/Linux
system, and then attempt to "run" a RISC-V binary using the native
x86-64 Linux target.  Previously this would trigger an assert while
accessing fields within a i386_gdbarch_tdep, like this:

  ../../src/gdb/i387-tdep.c:596: internal-error: i387_supply_fxsave: Assertion `tdep->st0_regnum >= I386_ST0_REGNUM' failed.

But with the changes from this commit in place, we now see an
assertion failure like this:

  ../../src/gdb/gdbarch.h:166: internal-error: gdbarch_tdep: Assertion `dynamic_cast<TDepType *> (tdep) != nullptr' failed.

On the face of it, this might not seem like much of an improvement,
but I think it is.

The previous assert was triggered by undefined behaviour.  There's no
guarantee that we would see an assertion at all, a different
combination of native target and binary format might not trigger an
assert (and just do the wrong thing), or might crash GDB completely.

In contrast, the new assert is based on defined behaviour, we'll
always assert if GDB goes wrong, and we assert early, at the point the
mistake is being made (casting the result of gdbarch_tdep to the wrong
type), rather than at some later point after the incorrect cast has
completed.

Obviously, when we consider the original example, trying to run a
binary of the wrong architecture on a native target, having GDB fail
with an assertion is not a real solution.  No user action should be
able to trigger an assertion failure.  In a later commit I will offer
a real solution to this architecture mismatch problem.
---
 gdb/gdbarch.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
index b2c91db0c4f..ea507d70ec9 100644
--- a/gdb/gdbarch.h
+++ b/gdb/gdbarch.h
@@ -58,7 +58,13 @@ struct inferior;
 
 #include "regcache.h"
 
-struct gdbarch_tdep {};
+/* The base class for every architecture's tdep sub-class.  We include a
+   virtual destructor so that sub-classes will have RTTI information.  */
+
+struct gdbarch_tdep
+{
+  virtual ~gdbarch_tdep() = default;
+};
 
 /* The architecture associated with the inferior through the
    connection to the target.
@@ -157,6 +163,7 @@ static inline TDepType *
 gdbarch_tdep (struct gdbarch *gdbarch)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep_1 (gdbarch);
+  gdb_assert (dynamic_cast<TDepType *> (tdep) != nullptr);
   return static_cast<TDepType *> (tdep);
 }
 
-- 
2.25.4