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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

This looks great, thank you for considering and incorporating my
suggestions. I agree that a number of them would make the diff harder to
review and can be considered future improvements. But there are two
changes which IMHO don't change the diff very much and are important
when converting to C++. More below.

Also a couple of formatting nits.

Guinevere Larsen <guinevere@redhat.com> writes:

> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
> index afc1258c6a9..7be01ec2203 100644
> --- a/gdb/frame-unwind.c
> +++ b/gdb/frame-unwind.c
> @@ -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
> +{
> +  return stop_reason_p (this_frame, this_prologue_cache);
> +}
> +
> +/* This method just passes the parameters to the callback pointer.  */
> +void
> +frame_unwind_legacy::this_id (const frame_info_ptr &this_frame,
> +			      void **this_prologue_cache,
> +			      struct frame_id *id) const
> +{
> +  return this_id_p (this_frame, this_prologue_cache, id);
> +}
> +
> +/* This method just passes the parameters to the callback pointer.  */
> +struct value *
> +frame_unwind_legacy::prev_register (const frame_info_ptr &this_frame,
> +				    void **this_prologue_cache,
> +				    int regnum) const
> +{
> +  return prev_register_p (this_frame, this_prologue_cache, regnum);
> +}
> +
> +/* This method just passes the parameters to the callback pointer.  */
> +int
> +frame_unwind_legacy::sniffer (const struct frame_unwind *self,
> +			      const frame_info_ptr &this_frame,
> +			      void **this_prologue_cache) const
> +{
> +  return sniffer_p (self, this_frame, this_prologue_cache);
> +}
> +
> +/* This method just passes the parameters to the callback pointer.  */
> +void
> +frame_unwind_legacy::dealloc_cache (frame_info *self, void *this_cache) const
> +{
> +  if (dealloc_cache_p != nullptr)
> +    dealloc_cache_p (self, this_cache);
> +}
> +
> +/* This method just passes the parameters to the callback pointer.  */
> +struct gdbarch *
> +frame_unwind_legacy::prev_arch (const frame_info_ptr &this_frame,
> +				void **this_prologue_cache) const
> +{
> +  if (prev_arch_p == nullptr)
> +    error (_("No prev_arch callback installed"));
> +  return prev_arch_p (this_frame, this_prologue_cache);
> +}

As I mentioned before, changing this to:

struct gdbarch *
frame_unwind_legacy::prev_arch (const frame_info_ptr &this_frame,
				void **this_prologue_cache) const
{
  if (prev_arch_p == nullptr)
    return frame_unwind::prev_arch (this_frame, this_prologue_cache);
  return prev_arch_p (this_frame, this_prologue_cache);
}

and frame_unwind::prev_arch () to:

virtual struct gdbarch *prev_arch (const frame_info_ptr &this_frame,
				   void **this_prologue_cache) const
{
  return get_frame_arch (this_frame);
}

Allows calling this method without the try/catch in frame_unwind_arch
().  When class inheritance provides an adequate solution to method
dispatch, I don't see a reason to throw and catch an exception to
accomplish the same task.

Also, the diff is simple:

https://git.linaro.org/people/thiago.bauermann/binutils-gdb.git/commit/?h=review/modernize-frame-unwinders&id=88db6b3f96c3afe7b3f45525b7f786d14a89bbc5

And (I could be wrong of course but) I don't see a risk for regressions,
so I don't see a need to leave this change for a future patch.

> 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;
> +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;
> +  }
> +
> +  enum frame_type type () const
> +  {
> +    return m_type;
> +  }
> +
> +  enum frame_unwind_class unwinder_class () const
> +  {
> +    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.  */
> +  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,
> +		       const frame_info_ptr &this_frame,
> +		       void **this_prologue_cache) const
> +  {
> +    return 1;
> +  }

The other thing I believe should be addressed in a C++ conversion is to
avoid having a method argument which duplicates the implicit 'this'
argument, as is the case of the 'self' argument in the sniffer ()
method.

Fortunately the diff is also small:

https://git.linaro.org/people/thiago.bauermann/binutils-gdb.git/commit/?h=review/modernize-frame-unwinders&id=9555b3ae4feea0f4ff15028257716ff2c3c42732

So IMHO it can be done in this patch.

Also, a nit: the lines starting with "const ..." and "void ..." above
are unaligned.

> 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;
> +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)
> +  { }
> +
> +  int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame,
> +	      void **this_prologue_cache) const override;

Space before '('.

> +
> +  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);
> +  }

With the prev_arch () changes I'm suggesting, this error () can be
changed to internal_error ().

> +
> +  /* 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
> +  { }
> +};

-- 
Thiago