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

[Guinevere Larsen <guinevere@redhat.com>]

On 10/3/24 5:06 PM, Simon Marchi wrote:
>> @@ -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.  */
Fixed.
>
>> 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.
Fixed.
>
>> +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...
I do prefer this style, so I went with it for all places where it would 
be under 80 columns.
>
>> +
>> +  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.
Fixed.
>
>> +  {
>> +    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"
I went with the second.
>
>> +  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).

Fixed.

I was reticent of doing it because I thought I'd need to change other 
sniffers, it didn't occur to me that I could pass "this" from inside the 
frame_unwind_legacy.

>
>> +		       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?
Yeah, this sounds like a reasonable further improvement. I just worry 
about changing many in a single commit, so I'll leave it for a future 
series.
>
>> +
>> +  /* 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.
I'll defer to your expertise on this. I have no actual idea how the 
cache stuff works in here, so if you say that this is a reasonable 
implementation and my regression tests didn't show any issues, I'm happy 
to go along with it.
>
>> +  }
>> +
>> +  /* 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.
Ok, fixed.
>
>
>> +  }
>> +};
>> +
>> +/* 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.

Huh... I thought I had seen a few parameters and variables that seemed 
to use _p for pointer. Fixed to use m_ instead.

>
>> 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.
Fixed.
>
>> +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]
Thiago agrees that no other frame unwinder uses frame_data, so I think 
they should probably be fixed up. However, again, I worry about too many 
actual changes apart from migrating to classes, so I think this is 
better suited for a follow up series.
>
>> +
>> +  int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame,
> Missing space.
fixed.
>
>> +	      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.
Ok, removed the fixme, and the method as a whole, so the base 
dealloc_cache implementation is used.
>
>> @@ -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
>
I'll work on your second email tomorrow, and hopefully be able to send a 
v6 with all fixes by the end of the week.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers