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

[Simon Marchi <simark@simark.ca>]

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