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

Simon Marchi
2024-10-04T05:21:39Z

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".

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.

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.

>> +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