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

Guinevere Larsen
2024-10-09T18:17:09Z

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