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

[Guinevere Larsen <guinevere@redhat.com>]

On 10/2/24 9:23 PM, Thiago Jung Bauermann wrote:
> 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.

Hi Thiago,

I was apparently over-complicating the changes. You're right, they are 
simple enough to add to this patch. I fixed everything you mentioned here.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

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