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

[Thiago Jung Bauermann]

Guinevere Larsen <guinevere@redhat.com> writes:

> On 9/27/24 12:51 AM, Thiago Jung Bauermann wrote:
>> Hello Guinevere,
>>
>> Great patch! I got carried away with it, and implemented all of the
>> suggestions I made to see whether they were feasible. If you want to
>> check how they affect the code, they're available in branch
>> 'review/modernize-frame-unwinders' on this repo:
>>
>> https://git.linaro.org/people/thiago.bauermann/binutils-gdb.git/
>>
>> Some of them IMHO can be incorporated in this patch, but some of them
>> would be better as separate patches. If you want, you can cherry-pick
>> them into your series, or I can post them later after it gets
>> in. Whichever way you prefer.
>
> Thank you for the thorough review!

No problem. Thanks for the patch series!

I forgot to mention: of course also feel free to squash any commit from
my branch into your patches. They're just small adjustments, so no need
to add 'co-authored-by' or anything.

>> Guinevere Larsen <blarsen@redhat.com> writes:
>>
>>> +/* 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);
>>> +}
>> Many frame_unwind_legacy instances use default_frame_sniffer, which
>> simply returns 1. Since this patch already defines an equivalent
>> frame_unwind::sniffer (), it's possible to remove the need of specifying
>> default_frame_sniffer in the declarations of many
>> frame_uwnind_legacy instances if this method is made to call the parent
>> class' implementation if sniffer_p is nullptr:
>>
>> /* This method just passes the parameters to the callback pointer.  */
>> int
>> frame_unwind_legacy::sniffer (const frame_info_ptr &this_frame,
>> 			      void **this_prologue_cache) const
>> {
>>    if (sniffer_p == nullptr)
>>      return frame_unwind::sniffer (this_frame, this_prologue_cache);
>>    return sniffer_p (this, this_frame, this_prologue_cache);
>> }
> Same as above, I agree that this is a great change but I worried it would make for a
> harder review.

That's true. It could be a follow-on patch. Like I said, I don't mind
posting one after your series goes in if you prefer.

>>> +  /* 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.  */
>> Rather than throwing an error at runtime to check if an unwinder has
>> implemented the desired functionality, I believe it's better to do the
>> check at compile time by making these methods pure virtual.
>>
>> This can be done to this_id () and prev_register (). prev_arch () can be
>> turned into a useful default implementation by making it call
>> get_frame_arch () as I mentioned before.
>>
>> This leaves just dealloc_cache (), so this comment can be moved to it.
>
> I remember trying to make the methods pure-virtual and getting linker errors when testing
> locally.
>
> I know that after that I found one unwinder that didn't set one of the pointers, so maybe
> that was fixed... If you managed to build with --enable-targets=all, I think we're safe.

Yes, I build-tested with --enable-targets=all. The only caveat is that I
don't have liunwind-ia64 so some of the unwinders in ia64-tdep.c are
untested — though I don't see how they could go wrong (famous last words).

>>> +/* 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;
>>> +public:
>>> +  frame_unwind_legacy (const char *n, frame_type t, frame_unwind_class c,
>>> +		       frame_unwind_stop_reason_ftype *sr,
>> As mentioned before, if this argument is moved later, it can be made
>> optional. Probably best done in a separate patch, if you agree with the
>> change.
>
> Makes sense
>
>>> +		       frame_this_id_ftype *ti,
>>> +		       frame_prev_register_ftype *pr,
>>> +		       const struct frame_data *ud,
>> It turns out that no frame_unwind_legacy instance passes a frame_data
>> object, so this argument can simply be removed and nullptr can be passed
>> to the parent class' constructor. Not sure if this is better done as a
>> separate patch, or as part of this patch.
>
> Oh, thanks for noticing that!
>
> I think it's  better to have it separate so we minimize things that can go wrong in one
> single change...

Agreed.

-- 
Thiago