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

Guinevere Larsen
2024-10-09T20:00:36Z

On 10/3/24 5:06 PM, Simon Marchi wrote:
>> @@ -332,6 +332,59 @@ frame_unwind_got_address (const frame_info_ptr &frame, int regnum,
>> return reg_val;
>> }
>>
>> +/* This method just passes the parameters to the callback pointer. */
>> +enum unwind_stop_reason
>> +frame_unwind_legacy::stop_reason (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const
> Since these methods are already documented in the .h, the comment should
> just say:
>
> /* See frame-unwind.h. */
Fixed.
>
>> diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
>> index deab4f7dbfb..b078945a200 100644
>> --- a/gdb/frame-unwind.h
>> +++ b/gdb/frame-unwind.h
>> @@ -169,25 +169,148 @@ enum frame_unwind_class
>> FRAME_UNWIND_ARCH,
>> };
>>
>> -struct frame_unwind
>> +class frame_unwind
>> {
>> - const char *name;
>> + const char *m_name;
>> /* The frame's type. Should this instead be a collection of
>> predicates that test the frame for various attributes? */
>> - enum frame_type type;
>> + enum frame_type m_type;
>> /* What kind of unwinder is this. It generally follows from where
>> the unwinder was added or where it looks for information to do the
>> unwinding. */
>> - enum frame_unwind_class unwinder_class;
>> + enum frame_unwind_class m_unwinder_class;
>> + const struct frame_data *m_unwind_data;
> To be consistent, please move the private fields at the end, like we do elsewhere.
Fixed.
>
>> +public:
>> + frame_unwind (const char *name, frame_type type, frame_unwind_class uclass,
>> + const struct frame_data *data)
>> + : m_name (name), m_type (type), m_unwinder_class (uclass),
>> + m_unwind_data (data)
>> + { }
>> +
>> + const char *name () const
>> + {
>> + return m_name;
>> + }
> If you want to save some lines, we use this style elsewhere for trivial
> getters:
>
> const char *name () const
> { return m_name; }
>
> I don't really mind if you prefer not, we currently have both styles in
> the code base...
I do prefer this style, so I went with it for all places where it would
be under 80 columns.
>
>> +
>> + enum frame_type type () const
>> + {
>> + return m_type;
>> + }
>> +
>> + enum frame_unwind_class unwinder_class () const
> To be more C++-y, I would suggest dropping enum/struct/class keywords
> where possible, for instance the return value above.
Fixed.
>
>> + {
>> + return m_unwinder_class;
>> + }
>> +
>> + const struct frame_data *unwind_data () const
>> + {
>> + return m_unwind_data;
>> + }
>> +
>> + /* Default stop_reason function. It reports NO_REASON, unless the
>> + frame is the outermost. */
> function -> method, I would say. Or "Default stop_reason
> implementation"
I went with the second.
>
>> + virtual enum unwind_stop_reason stop_reason (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const
>> + {
>> + return default_frame_unwind_stop_reason (this_frame, this_prologue_cache);
>> + }
>> +
>> + /* Default frame sniffer. Will always return that the unwinder
>> + is able to unwind the frame. */
>> + virtual int sniffer (const frame_unwind *self,
> I agree with Thiago's comment about not passing "self" (unless there's
> something I don't see here).

Fixed.

I was reticent of doing it because I thought I'd need to change other
sniffers, it didn't occur to me that I could pass "this" from inside the
frame_unwind_legacy.

>
>> + const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const
>> + {
>> + return 1;
>> + }
>> +
>> + /* Calculate the ID of the given frame using this unwinder. */
>> + virtual void this_id (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache,
>> + struct frame_id *id) const = 0;
> This is pre-existing code so it makes sense to have it like that for the
> moment, but I don't see why this has to return the value by parameter,
> surely the method could return a frame_id directly?
Yeah, this sounds like a reasonable further improvement. I just worry
about changing many in a single commit, so I'll leave it for a future
series.
>
>> +
>> + /* Get the value of a register in a previous frame. */
>> + virtual struct value *prev_register (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache,
>> + int regnum) const = 0;
>> +
>> + /* 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. */
>> +
>> + /* Properly deallocate the cache. */
>> + virtual void dealloc_cache (frame_info *self, void *this_cache) const
>> + {
>> + internal_error (_("No method dealloc_cache implemented for unwinder %s"),
>> + m_name);
> I think a reasonable default implementation here would be to do nothing.
> After all, that's what the current code (function frame_info_del) does:
> if the callback is nullptr, nothing is done. An implementation that
> doesn't use any frame cache (is there any?) will just be able to omit
> this method.
I'll defer to your expertise on this. I have no actual idea how the
cache stuff works in here, so if you say that this is a reasonable
implementation and my regression tests didn't show any issues, I'm happy
to go along with it.
>
>> + }
>> +
>> + /* Get the previous architecture. */
>> + virtual struct gdbarch *prev_arch (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const
>> + {
>> + error (_("No method prev_arch implemented for unwinder %s"), m_name);
> I think Thiago's suggestion makes sense, a default implementation of
> returning this_frame's arch would be reasonable. It is by far the most
> common and intuitive case that the arch for a given frame will be the
> same as the arch for the frame it got unwound from. Only the few
> implementations that want to do something different (e.g. sentinel) can
> implement it.
Ok, fixed.
>
>
>> + }
>> +};
>> +
>> +/* 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;
> Move the private fields at the end, and prefix with `m_`. Also, in GNU
> code the `_p` prefix usually means "predicate", so usually for boolean
> values or functions returning booleans. I guess here with `_p` you
> meant "pointer". Just doing with "m_stop_reason" & co would be fine I
> think, it's not worse than what was there previously.

Huh... I thought I had seen a few parameters and variables that seemed
to use _p for pointer. Fixed to use m_ instead.

>
>> diff --git a/gdb/tramp-frame.c b/gdb/tramp-frame.c
>> index 6368f67a2e7..b1f3be66bc1 100644
>> --- a/gdb/tramp-frame.c
>> +++ b/gdb/tramp-frame.c
>> @@ -57,10 +57,45 @@ tramp_frame_cache (const frame_info_ptr &this_frame,
>> return tramp_cache->trad_cache;
>> }
>>
>> -static void
>> -tramp_frame_this_id (const frame_info_ptr &this_frame,
>> - void **this_cache,
>> - struct frame_id *this_id)
>> +class frame_unwind_trampoline : public frame_unwind
>> +{
>> +private:
>> + frame_prev_arch_ftype *prev_arch_p;
> Please move this at the end of the class. And I suggest naming it
> "m_prev_arch", for the same reason as explained above.
Fixed.
>
>> +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]
Thiago agrees that no other frame unwinder uses frame_data, so I think
they should probably be fixed up. However, again, I worry about too many
actual changes apart from migrating to classes, so I think this is
better suited for a follow up series.
>
>> +
>> + int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame,
> Missing space.
fixed.
>
>> + void **this_prologue_cache) const override;
>> +
>> + void this_id (const frame_info_ptr &this_frame, void **this_prologue_cache,
>> + struct frame_id *id) const override;
>> +
>> + struct value *prev_register (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache,
>> + int regnum) const override;
>> +
>> + struct gdbarch *prev_arch (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const override
>> + {
>> + if (prev_arch_p == nullptr)
>> + error (_("No prev_arch callback installed"));
>> + return prev_arch_p (this_frame, this_prologue_cache);
>> + }
>> +
>> + /* FIXME: This should have a proper algorithm to deallocate the cache,
>> + otherwise memory is leaked. This method is empty here just so the
>> + migration to c++ classes doesn't add regressions. */
>> + void dealloc_cache (frame_info *self, void *this_cache) const override
>> + { }
> From what I can see, this unwinder uses the "trad frame" cache, which
> allocates everything on the frame_cache_obstack obstack, which gets
> cleared in reinit_frame_cache(). So I think this unwinder could just
> omit the dealloc_cache method, as it would not need it.
Ok, removed the fixme, and the method as a whole, so the base
dealloc_cache implementation is used.
>
>> @@ -161,16 +196,11 @@ tramp_frame_prepend_unwinder (struct gdbarch *gdbarch,
>> gdb_assert (tramp_frame->insn_size <= sizeof (tramp_frame->insn[0].bytes));
>>
>> data = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_data);
>> - unwinder = GDBARCH_OBSTACK_ZALLOC (gdbarch, struct frame_unwind);
>> -
>> data->tramp_frame = tramp_frame;
>> - unwinder->type = tramp_frame->frame_type;
>> - unwinder->unwind_data = data;
>> - unwinder->unwinder_class = FRAME_UNWIND_GDB;
>> - unwinder->sniffer = tramp_frame_sniffer;
>> - unwinder->stop_reason = default_frame_unwind_stop_reason;
>> - unwinder->this_id = tramp_frame_this_id;
>> - unwinder->prev_register = tramp_frame_prev_register;
>> - unwinder->prev_arch = tramp_frame->prev_arch;
>> +
>> + unwinder = obstack_new <frame_unwind_trampoline> (gdbarch_obstack (gdbarch),
>> + tramp_frame->frame_type,
>> + data,
>> + tramp_frame->prev_arch);
> Here, given that `data` points to the `frame_data` object, which
> contains a pointer to the `tramp_frame`, I think it's redundant to pass
> that in addition to `frame_type` and `prev_arch`. I think you could
> replace all that by a single argument of type `tramp_frame *` (and store
> that pointer directly in the `frame_unwind_trampoline` object, instead
> of passing through that `frame_data` thing).
>
> Simon
>
I'll work on your second email tomorrow, and hopefully be able to send a
v6 with all fixes by the end of the week.

--
Cheers,
Guinevere Larsen
She/Her/Hers