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

[Simon Marchi <simark@simark.ca>]

> @@ -332,6 +332,59 @@ frame_unwind_got_address (const frame_info_ptr &frame, int regnum,
>    return reg_val;
>  }
>  
> +/* This method just passes the parameters to the callback pointer.  */
> +enum unwind_stop_reason
> +frame_unwind_legacy::stop_reason (const frame_info_ptr &this_frame,
> +				  void **this_prologue_cache) const

Since these methods are already documented in the .h, the comment should
just say:

/* See frame-unwind.h.  */

> diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
> index deab4f7dbfb..b078945a200 100644
> --- a/gdb/frame-unwind.h
> +++ b/gdb/frame-unwind.h
> @@ -169,25 +169,148 @@ enum frame_unwind_class
>    FRAME_UNWIND_ARCH,
>  };
>  
> -struct frame_unwind
> +class frame_unwind
>  {
> -  const char *name;
> +  const char *m_name;
>    /* The frame's type.  Should this instead be a collection of
>       predicates that test the frame for various attributes?  */
> -  enum frame_type type;
> +  enum frame_type m_type;
>    /* What kind of unwinder is this.  It generally follows from where
>       the unwinder was added or where it looks for information to do the
>       unwinding.  */
> -  enum frame_unwind_class unwinder_class;
> +  enum frame_unwind_class m_unwinder_class;
> +  const struct frame_data *m_unwind_data;

To be consistent, please move the private fields at the end, like we do elsewhere.

> +public:
> +  frame_unwind (const char *name, frame_type type, frame_unwind_class uclass,
> +		       const struct frame_data *data)
> +    : m_name (name), m_type (type), m_unwinder_class (uclass),
> +      m_unwind_data (data)
> +    { }
> +
> +  const char *name () const
> +  {
> +    return m_name;
> +  }

If you want to save some lines, we use this style elsewhere for trivial
getters:

const char *name () const
{ return m_name; }

I don't really mind if you prefer not, we currently have both styles in
the code base...

> +
> +  enum frame_type type () const
> +  {
> +    return m_type;
> +  }
> +
> +  enum frame_unwind_class unwinder_class () const

To be more C++-y, I would suggest dropping enum/struct/class keywords
where possible, for instance the return value above.

> +  {
> +    return m_unwinder_class;
> +  }
> +
> +  const struct frame_data *unwind_data () const
> +  {
> +    return m_unwind_data;
> +  }
> +
> +  /* Default stop_reason function.  It reports NO_REASON, unless the
> +     frame is the outermost.  */

function -> method, I would say.  Or "Default stop_reason
implementation"

> +  virtual enum unwind_stop_reason stop_reason (const frame_info_ptr &this_frame,
> +					       void **this_prologue_cache) const
> +  {
> +    return default_frame_unwind_stop_reason (this_frame, this_prologue_cache);
> +  }
> +
> +  /* Default frame sniffer.  Will always return that the unwinder
> +     is able to unwind the frame.  */
> +  virtual int sniffer (const frame_unwind *self,

I agree with Thiago's comment about not passing "self" (unless there's
something I don't see here).

> +		       const frame_info_ptr &this_frame,
> +		       void **this_prologue_cache) const
> +  {
> +    return 1;
> +  }
> +
> +  /* 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 = 0;

This is pre-existing code so it makes sense to have it like that for the
moment, but I don't see why this has to return the value by parameter,
surely the method could return a frame_id directly?

> +
> +  /* Get the value of a register in a previous frame.  */
> +  virtual struct value *prev_register (const frame_info_ptr &this_frame,
> +				       void **this_prologue_cache,
> +				       int regnum) const = 0;
> +
> +  /* The following methods are here mostly for interface functionality.  They
> +     all throw an error when called, as a safe way to check if an unwinder has
> +     implemented the desired functionality.  */
> +
> +  /* Properly deallocate the cache.  */
> +  virtual void dealloc_cache (frame_info *self, void *this_cache) const
> +  {
> +    internal_error (_("No method dealloc_cache implemented for unwinder %s"),
> +		    m_name);

I think a reasonable default implementation here would be to do nothing.
After all, that's what the current code (function frame_info_del) does:
if the callback is nullptr, nothing is done.  An implementation that
doesn't use any frame cache (is there any?) will just be able to omit
this method.

> +  }
> +
> +  /* Get the previous architecture.  */
> +  virtual struct gdbarch *prev_arch (const frame_info_ptr &this_frame,
> +				     void **this_prologue_cache) const
> +  {
> +    error (_("No method prev_arch implemented for unwinder %s"), m_name);

I think Thiago's suggestion makes sense, a default implementation of
returning this_frame's arch would be reasonable.  It is by far the most
common and intuitive case that the arch for a given frame will be the
same as the arch for the frame it got unwound from.  Only the few
implementations that want to do something different (e.g. sentinel) can
implement it.


> +  }
> +};
> +
> +/* This is a legacy version of the frame unwinder.  The original struct
> +   used function pointers for callbacks, this updated version has a
> +   method that just passes the parameters along to the callback
> +   pointer.
> +   Do not use this class for new unwinders.  Instead, see other classes
> +   that inherit from frame_unwind, such as the python unwinder.  */
> +class frame_unwind_legacy : public frame_unwind
> +{
> +private:
>    /* Should an attribute indicating the frame's address-in-block go
>       here?  */
> -  frame_unwind_stop_reason_ftype *stop_reason;
> -  frame_this_id_ftype *this_id;
> -  frame_prev_register_ftype *prev_register;
> -  const struct frame_data *unwind_data;
> -  frame_sniffer_ftype *sniffer;
> -  frame_dealloc_cache_ftype *dealloc_cache;
> -  frame_prev_arch_ftype *prev_arch;
> +  frame_unwind_stop_reason_ftype *stop_reason_p;
> +  frame_this_id_ftype *this_id_p;
> +  frame_prev_register_ftype *prev_register_p;
> +  frame_sniffer_ftype *sniffer_p;
> +  frame_dealloc_cache_ftype *dealloc_cache_p;
> +  frame_prev_arch_ftype *prev_arch_p;

Move the private fields at the end, and prefix with `m_`.  Also, in GNU
code the `_p` prefix usually means "predicate", so usually for boolean
values or functions returning booleans.  I guess here with `_p` you
meant "pointer".  Just doing with "m_stop_reason" & co would be fine I
think, it's not worse than what was there previously.

> diff --git a/gdb/tramp-frame.c b/gdb/tramp-frame.c
> index 6368f67a2e7..b1f3be66bc1 100644
> --- a/gdb/tramp-frame.c
> +++ b/gdb/tramp-frame.c
> @@ -57,10 +57,45 @@ tramp_frame_cache (const frame_info_ptr &this_frame,
>    return tramp_cache->trad_cache;
>  }
>  
> -static void
> -tramp_frame_this_id (const frame_info_ptr &this_frame,
> -		     void **this_cache,
> -		     struct frame_id *this_id)
> +class frame_unwind_trampoline : public frame_unwind
> +{
> +private:
> +  frame_prev_arch_ftype *prev_arch_p;

Please move this at the end of the class.  And I suggest naming it
"m_prev_arch", for the same reason as explained above.

> +public:
> +  frame_unwind_trampoline (enum frame_type t, const struct frame_data *d,
> +			   frame_prev_arch_ftype *pa)
> +    : frame_unwind ("trampoline", t, FRAME_UNWIND_GDB, d), prev_arch_p (pa)
> +  { }

This "frame_data" thing appears to be some kind of opaque / private data
that frame_unwind implementations can use?  For those that you converted
to proper classes (trampoline and python), could that data be stored in
the frame_unwind_* object directly?  It would remove some unnecessary
indirections and make things simpler, I think.

Actually, I gave it a try, and trampoline and python are the only
implementations actually using the frame_data pointer.  So if they stop
using it, we can just get rid of that.  That requires touching all the
frame_unwind_legacy constructor call sites, but I was able to do that
relatively easily with a global regex find & replace in VScode. [last
minute edit: I just realized that I was building without
--enable-targets=all, so take this with a grain of salt]

> +
> +  int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame,

Missing space.

> +	      void **this_prologue_cache) const override;
> +
> +  void this_id (const frame_info_ptr &this_frame, void **this_prologue_cache,
> +		struct frame_id *id) const override;
> +
> +  struct value *prev_register (const frame_info_ptr &this_frame,
> +			       void **this_prologue_cache,
> +			       int regnum) const override;
> +
> +  struct gdbarch *prev_arch (const frame_info_ptr &this_frame,
> +			     void **this_prologue_cache) const override
> +  {
> +    if (prev_arch_p == nullptr)
> +      error (_("No prev_arch callback installed"));
> +    return prev_arch_p (this_frame, this_prologue_cache);
> +  }
> +
> +  /* FIXME: This should have a proper algorithm to deallocate the cache,
> +     otherwise memory is leaked.  This method is empty here just so the
> +     migration to c++ classes doesn't add regressions.  */
> +  void dealloc_cache (frame_info *self, void *this_cache) const override
> +  { }

From what I can see, this unwinder uses the "trad frame" cache, which
allocates everything on the frame_cache_obstack obstack, which gets
cleared in reinit_frame_cache().  So I think this unwinder could just
omit the dealloc_cache method, as it would not need it.

> @@ -161,16 +196,11 @@ tramp_frame_prepend_unwinder (struct gdbarch *gdbarch,
>    gdb_assert (tramp_frame->insn_size <= sizeof (tramp_frame->insn[0].bytes));
>  
>    data = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_data);
> -  unwinder = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind);
> -
>    data->tramp_frame = tramp_frame;
> -  unwinder->type = tramp_frame->frame_type;
> -  unwinder->unwind_data = data;
> -  unwinder->unwinder_class = FRAME_UNWIND_GDB;
> -  unwinder->sniffer = tramp_frame_sniffer;
> -  unwinder->stop_reason = default_frame_unwind_stop_reason;
> -  unwinder->this_id = tramp_frame_this_id;
> -  unwinder->prev_register = tramp_frame_prev_register;
> -  unwinder->prev_arch = tramp_frame->prev_arch;
> +
> +  unwinder = obstack_new <frame_unwind_trampoline> (gdbarch_obstack (gdbarch),
> +						    tramp_frame->frame_type,
> +						    data,
> +						    tramp_frame->prev_arch);

Here, given that `data` points to the `frame_data` object, which
contains a pointer to the `tramp_frame`, I think it's redundant to pass
that in addition to `frame_type` and `prev_arch`.  I think you could
replace all that by a single argument of type `tramp_frame *` (and store
that pointer directly in the `frame_unwind_trampoline` object, instead
of passing through that `frame_data` thing).

Simon