Re: [PATCHv3 5/6] gdb: ensure the cast in gdbarch_tdep is valid

[Pedro Alves <pedro@palves.net>]

On 2022-06-13 17:15, 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.

I don't think that's true.  dynamic_cast does not really do a vtable
pointer comparison.  It does pointer arithmetic/adjustment, walks the vtables
to find the type info for each type in the hierarchy, and compares the type's
type info objects, which usually mean string-comparing the class names.  Only
some systems can do pointer comparisons to compare types.  See e.g.:

  https://github.com/gcc-mirror/gcc/blob/bb8e93eb1acae30a5fbe7e13149493ce4ccd301a/libstdc%2B%2B-v3/libsupc%2B%2B/typeinfo#L52

See the benchmark I posted in this previous response to Tromey:

  https://sourceware.org/pipermail/gdb-patches/2022-April/188102.html

Maybe we should revisit Tromey's idea of a new cast "operator",
but make it static_cast in release mode?  This patch that I posted today
adds a DEVELOPMENT macro symbol to common-config.h, we can make use of that:

  https://sourceware.org/pipermail/gdb-patches/2022-June/190301.html

Also, dynamic_cast lets you try to cast to completely unrelated types (undefined
behavior instead of compile error), while to me is seems better to make it fail
to compile.  Like:

template<typename T, typename V>
T *
checked_dyn_cast (V *v)
{
  /* dynamic_cast compiles even if V and T are completely unrelated.
     This assert prevent that, only letting you do downcasts, and
     casts to same type.  */
  static_assert (std::is_base_of<V, T>::value
		 || std::is_base_of<T, V>::value,
		 "types must be related");
  /* Check for polymorphism explicitly in case we're in release
     mode.  */
  static_assert (std::is_polymorphic<V>::value,
		 "types must be polymorphic");

#ifdef DEVELOPMENT
  T *result = dynamic_cast<T *> (v);
  assert (result != nullptr);
#else
  T *result = static_cast<T *> (v);
#endif
  return result;
}

With that, this compiles & works as expected:

struct A
{
  virtual ~A() {}
};

struct B : A {};

struct C {};

void func () 
{
  A *a = new A();
  B *b = new B();
  C *c = new C();

  // these work
  checked_dyn_cast<A> (a);
  checked_dyn_cast<B> (a);
  checked_dyn_cast<A> (b);
  checked_dyn_cast<const B> (a);

  // while these fail the static assertions.
  // checked_dyn_cast<C> (a);
  // checked_dyn_cast<A> (c);
  // checked_dyn_cast<A> (123);
}

Anyhow, this can always be done after your patch, I don't mean to suggest
we should do this before.

Other than the nit in the commit log, the patch LGTM.