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

Tom Tromey
2024-03-08T17:07:31Z

>>>>> Guinevere Larsen <blarsen@redhat.com> writes:

> Frame unwinders have historically been a structure populated with
> callback pointers, so that architectures (or other specific unwinders)
> could install their own way to handle the inferior. However, since
> moving to c++, we could use polymorphism to get the same functionality
> in a more readable way. Polymorphism also makes it simpler to add new
> functionality to all frame unwinders, since all that's required is
> adding it to the base class.

> As part of the changes to add support to disabling frame unwinders,
> this commit makes the first baby step in using polymorphism for the
> frame unwinders, by making frame_unwind a virtual class, and adds a
> couple of new classes. The main class added is frame_unwind_legacy,
> which works the same as the previous structs, using function pointers
> as callbacks. This class was added to allow the transition to happen
> piecemeal. New unwinders should instead follow the lead of the other
> classes implemented.

> 2 of the others, frame_unwind_python and frame_unwind_trampoline, were added
> because it seemed simpler at the moment to do that instead of reworking
> the dynamic allocation to work with the legacy class, and can be used as
> an example to future implementations.

> /* AArch64 prologue unwinder. */
> -static frame_unwind aarch64_prologue_unwind =
> -{
> +static frame_unwind_legacy aarch64_prologue_unwind (

Some of these should probably be 'const'... not really your problem but
the patch points it out.

> +/* See frame-unwind.h. */
> +
> +enum unwind_stop_reason
> +frame_unwind::stop_reason (const frame_info_ptr &this_frame,
> + void **this_prologue_cache) const
> +{
> + return default_frame_unwind_stop_reason (this_frame, this_prologue_cache);
> +}
> +

It looks like all of these methods here could just be inline in the
header.

> -struct frame_unwind
> +class frame_unwind
> {
> - const char *name;
> +private:

class defaults to private.

> + frame_unwind (const char *n, frame_type t, frame_unwind_class c,
> + const struct frame_data *d)
> + : m_name (n), m_type (t), m_unwinder_class (c), m_unwind_data (d) { }

This looks weird.

> + /* 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
> + {
> + error (_("No method this_id implemented for unwinder %s"), m_name);
> + }

Why not pure virtual?

> + frame_unwind_legacy (const char *n, frame_type t, frame_unwind_class c,
> + frame_unwind_stop_reason_ftype *sr,
> + frame_this_id_ftype *ti,
> + frame_prev_register_ftype *pr,
> + const struct frame_data *ud,
> + frame_sniffer_ftype *s,
> + frame_dealloc_cache_ftype *dc = nullptr,
> + frame_prev_arch_ftype *pa = nullptr)
> + : frame_unwind (n, t, c, ud), stop_reason_p (sr),
> + this_id_p (ti), prev_register_p (pr), sniffer_p (s),
> + dealloc_cache_p (dc), prev_arch_p (pa) { }

I wonder if making this constexpr would help avoid running a ton of
constructors at startup time.

Alternatively maybe an approach would be to overload
frame_unwind_append_unwinder to automatically instantiate the frame_unwind_legacy
object given the C-style struct. I think this would delay
running the constructor until the gdbarch is initialized, which
in many cases would be never.

> + = obstack_new<frame_unwind_python>
> + (gdbarch_obstack(newarch), (const struct frame_data *) newarch);
> +

I think it's better to derive from allocate_on_obstack instead. This
helps prevent errors.

Also a missing space.

Tom