Re: [PATCH 3/4] gdb: Migrate frame unwinders to use C++ classes

[Tom Tromey <tom@tromey.com>]

>>>>> Guinevere Larsen <blarsen@redhat.com> writes:

> Frame unwinders have historically been a structure populated with
> callback pointers, so that architectures (or other specific unwinders)
> could install their own way to handle the inferior. However, since
> moving to c++, we could use polymorphism to get the same functionality
> in a more readable way. Polymorphism also makes it simpler to add new
> functionality to all frame unwinders, since all that's required is
> adding it to the base class.

> As part of the changes to add support to disabling frame unwinders,
> this commit makes the first baby step in  using polymorphism for the
> frame unwinders, by making frame_unwind a virtual class, and adds a
> couple of new classes. The main class added is frame_unwind_legacy,
> which works the same as the previous structs, using function pointers
> as callbacks. This class was added to allow the transition to happen
> piecemeal. New unwinders should instead follow the lead of the other
> classes implemented.

> 2 of the others, frame_unwind_python and frame_unwind_trampoline, were added
> because it seemed simpler at the moment to do that instead of reworking
> the dynamic allocation to work with the legacy class, and can be used as
> an example to future implementations.

>  /* AArch64 prologue unwinder.  */
> -static frame_unwind aarch64_prologue_unwind =
> -{
> +static frame_unwind_legacy aarch64_prologue_unwind (

Some of these should probably be 'const'...  not really your problem but
the patch points it out.

> +/* See frame-unwind.h.  */
> +
> +enum unwind_stop_reason
> +frame_unwind::stop_reason (const frame_info_ptr &this_frame,
> +			   void **this_prologue_cache) const
> +{
> +  return default_frame_unwind_stop_reason (this_frame, this_prologue_cache);
> +}
> +

It looks like all of these methods here could just be inline in the
header.

> -struct frame_unwind
> +class frame_unwind
>  {
> -  const char *name;
> +private:

class defaults to private.

> +  frame_unwind (const char *n, frame_type t, frame_unwind_class c,
> +		       const struct frame_data *d)
> +    : m_name (n), m_type (t), m_unwinder_class (c), m_unwind_data (d) { }

This looks weird.

> +  /* Calculate the ID of the given frame using this unwinder.  */
> +  virtual void this_id (const frame_info_ptr &this_frame,
> +			void **this_prologue_cache,
> +			struct frame_id *id) const
> +  {
> +    error (_("No method this_id implemented for unwinder %s"), m_name);
> +  }

Why not pure virtual?

> +  frame_unwind_legacy (const char *n, frame_type t, frame_unwind_class c,
> +		       frame_unwind_stop_reason_ftype *sr,
> +		       frame_this_id_ftype *ti,
> +		       frame_prev_register_ftype *pr,
> +		       const struct frame_data *ud,
> +		       frame_sniffer_ftype *s,
> +		       frame_dealloc_cache_ftype *dc = nullptr,
> +		       frame_prev_arch_ftype *pa = nullptr)
> +  : frame_unwind (n, t, c, ud), stop_reason_p (sr),
> +    this_id_p (ti), prev_register_p (pr), sniffer_p (s),
> +    dealloc_cache_p (dc), prev_arch_p (pa) { }

I wonder if making this constexpr would help avoid running a ton of
constructors at startup time.

Alternatively maybe an approach would be to overload
frame_unwind_append_unwinder to automatically instantiate the frame_unwind_legacy
object given the C-style struct.  I think this would delay
running the constructor until the gdbarch is initialized, which
in many cases would be never.

> +	  = obstack_new<frame_unwind_python>
> +	    (gdbarch_obstack(newarch), (const struct frame_data *) newarch);
> +

I think it's better to derive from allocate_on_obstack instead.  This
helps prevent errors.

Also a missing space.

Tom