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

[Guinevere Larsen <guinevere@redhat.com>]

On 10/4/24 2:21 AM, Simon Marchi wrote:
> Some more things I thought about...
>
>>> +  /* Default frame sniffer.  Will always return that the unwinder
>>> +     is able to unwind the frame.  */
>>> +  virtual int sniffer (const frame_unwind *self,
> One nit: I think this method should be called just "sniff".  When a
> method is named after a thing, my brain adds an implicit "get_" in front
> of it.  So to me, a method named "sniffer" would return some sniffer
> object.  Here, I think it would make more sense to use an action verb,
> because we want the unwinder to "sniff".
This makes sense, and is a pretty trivial change. Fixed.
>
> Oh and I guess another naming nit, I think frame_unwind should be named
> frame_unwinder.  A "thing" that unwinds stuff would be called an
> "unwinder", not an "unwind".  I don't know how you feel about changing
> that.

This is a reasonably big change, because several functions receive 
frame_unwind as a parameter.

If we progress conversion from the legacy to the specific classes we're 
going to reduce the number of function that get it as a parameter and it 
will turn into a smaller, more manageable patch. I think makes sense for 
future improvement.

>
> And something else I just remembered, I am not really a fan of the
> very abbreviated parameter names like this:
>
>    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)
>    { }
>
> When using an editor that provides tooltips when writing a function
> call, the abbreviated names in the tooltip are not very helpful.  I
> would suggest using proper names.
This makes sense. I really dislike having a function named the same as a 
type, but I will use "type", "data" and "prev_arch_func".
>>> +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]
> One more thought about all this: in an ideal world, the tramp_frame uses
> should probably become sub-classes of frame_unwind_trampoline.  Only
> those sites that want to override the prev_arch method would need to do
> it (AFAIK, the only one is aarch64_linux_rt_sigframe).  The `init` and
> `validate` callbacks in `tramp_frame` would become virtual methods of
> `frame_unwind_trampoline`, and the other data fields of `tramp_frame`
> would become fields of `frame_unwind_trampoline`.  I started to
> prototype it on top of this patch, and it would be quite a lot of work.
> So let's keep that on the wishlist for now.
>
> Simon
>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers