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

[John Baldwin <jhb@FreeBSD.org>]

On 5/31/22 7:30 AM, Andrew Burgess via Gdb-patches wrote:
> 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);
>   }

Maybe simplify to use dynamic_cast for the returned value, e.g.

{
   TDepType *tdep = dynamic_cast<TDepType *> (gdbarch_tdep_1 (gdbarch));
   gdb_assert(tdep != nullptr);
   return (tdep);
}

-- 
John Baldwin