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

Guinevere Larsen
2024-10-10T14:11:15Z

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