[PATCH v4 3/5] gdb: Migrate frame unwinders to use C++ classes
Guinevere Larsen
guinevere@redhat.com
Fri Sep 27 14:53:27 GMT 2024
On 9/27/24 12:51 AM, Thiago Jung Bauermann wrote:
> Hello Guinevere,
>
> Great patch! I got carried away with it, and implemented all of the
> suggestions I made to see whether they were feasible. If you want to
> check how they affect the code, they're available in branch
> 'review/modernize-frame-unwinders' on this repo:
>
> https://git.linaro.org/people/thiago.bauermann/binutils-gdb.git/
>
> Some of them IMHO can be incorporated in this patch, but some of them
> would be better as separate patches. If you want, you can cherry-pick
> them into your series, or I can post them later after it gets
> in. Whichever way you prefer.
Thank you for the thorough review!
>
> 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
> Nit: s/c++/C++/
>
>> 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.
>>
>> Finally, the cygwin unwinder was converted to a class since it was most
>> of the way there already.
>> ---
>> gdb/aarch64-tdep.c | 10 +--
>> gdb/alpha-mdebug-tdep.c | 5 +-
>> gdb/alpha-tdep.c | 10 +--
>> gdb/amd64-obsd-tdep.c | 5 +-
>> gdb/amd64-tdep.c | 20 ++---
>> gdb/amd64-windows-tdep.c | 5 +-
>> gdb/amdgpu-tdep.c | 6 +-
>> gdb/arc-tdep.c | 8 +-
>> gdb/arm-tdep.c | 24 +++---
>> gdb/avr-tdep.c | 4 +-
>> gdb/bfin-tdep.c | 5 +-
>> gdb/bpf-tdep.c | 5 +-
>> gdb/cris-tdep.c | 10 +--
>> gdb/csky-tdep.c | 8 +-
>> gdb/dummy-frame.c | 7 +-
>> gdb/dummy-frame.h | 2 +-
>> gdb/dwarf2/frame-tailcall.c | 5 +-
>> gdb/dwarf2/frame-tailcall.h | 2 +-
>> gdb/dwarf2/frame.c | 14 ++--
>> gdb/frame-unwind.c | 61 ++++++++++++++-
>> gdb/frame-unwind.h | 150 +++++++++++++++++++++++++++++++++---
>> gdb/frame.c | 28 +++----
>> gdb/frv-linux-tdep.c | 5 +-
>> gdb/frv-tdep.c | 4 +-
>> gdb/ft32-tdep.c | 5 +-
>> gdb/h8300-tdep.c | 4 +-
>> gdb/hppa-linux-tdep.c | 4 +-
>> gdb/hppa-tdep.c | 14 ++--
>> gdb/i386-obsd-tdep.c | 4 +-
>> gdb/i386-tdep.c | 25 +++---
>> gdb/ia64-tdep.c | 20 ++---
>> gdb/inline-frame.c | 4 +-
>> gdb/inline-frame.h | 2 +-
>> gdb/iq2000-tdep.c | 4 +-
>> gdb/jit.c | 5 +-
>> gdb/lm32-tdep.c | 4 +-
>> gdb/loongarch-tdep.c | 6 +-
>> gdb/m32c-tdep.c | 4 +-
>> gdb/m32r-linux-tdep.c | 4 +-
>> gdb/m32r-tdep.c | 4 +-
>> gdb/m68hc11-tdep.c | 4 +-
>> gdb/m68k-linux-tdep.c | 5 +-
>> gdb/m68k-tdep.c | 5 +-
>> gdb/mep-tdep.c | 4 +-
>> gdb/microblaze-tdep.c | 5 +-
>> gdb/mips-sde-tdep.c | 5 +-
>> gdb/mips-tdep.c | 20 ++---
>> gdb/mn10300-tdep.c | 4 +-
>> gdb/moxie-tdep.c | 4 +-
>> gdb/msp430-tdep.c | 4 +-
>> gdb/nds32-tdep.c | 12 ++-
>> gdb/nios2-tdep.c | 10 +--
>> gdb/or1k-tdep.c | 6 +-
>> gdb/ppc-fbsd-tdep.c | 4 +-
>> gdb/ppc-obsd-tdep.c | 4 +-
>> gdb/python/py-unwind.c | 51 +++++++++---
>> gdb/record-btrace.c | 10 +--
>> gdb/record.h | 4 +-
>> gdb/riscv-tdep.c | 7 +-
>> gdb/rl78-tdep.c | 5 +-
>> gdb/rs6000-aix-tdep.c | 4 +-
>> gdb/rs6000-tdep.c | 10 +--
>> gdb/rx-tdep.c | 8 +-
>> gdb/s12z-tdep.c | 6 +-
>> gdb/s390-linux-tdep.c | 4 +-
>> gdb/s390-tdep.c | 8 +-
>> gdb/sentinel-frame.c | 7 +-
>> gdb/sentinel-frame.h | 2 +-
>> gdb/sh-tdep.c | 9 +--
>> gdb/sparc-netbsd-tdep.c | 5 +-
>> gdb/sparc-obsd-tdep.c | 5 +-
>> gdb/sparc-sol2-tdep.c | 5 +-
>> gdb/sparc-tdep.c | 5 +-
>> gdb/sparc64-fbsd-tdep.c | 5 +-
>> gdb/sparc64-netbsd-tdep.c | 5 +-
>> gdb/sparc64-obsd-tdep.c | 10 +--
>> gdb/sparc64-sol2-tdep.c | 5 +-
>> gdb/sparc64-tdep.c | 5 +-
>> gdb/tic6x-tdep.c | 10 +--
>> gdb/tilegx-tdep.c | 4 +-
>> gdb/tramp-frame.c | 59 +++++++++++---
>> gdb/v850-tdep.c | 4 +-
>> gdb/vax-tdep.c | 5 +-
>> gdb/windows-tdep.c | 33 ++++----
>> gdb/windows-tdep.h | 16 +++-
>> gdb/xstormy16-tdep.c | 4 +-
>> gdb/xtensa-tdep.c | 6 +-
>> gdb/z80-tdep.c | 6 +-
>> 88 files changed, 563 insertions(+), 381 deletions(-)
> Working on this patch made me appreciate the sheer number of targets
> that GDB supports!
>
>> diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
>> index d867f0e152d..b79caffbd2d 100644
>> --- a/gdb/frame-unwind.c
>> +++ b/gdb/frame-unwind.c
>> @@ -129,7 +129,7 @@ frame_unwind_try_unwinder (const frame_info_ptr &this_frame, void **this_cache,
>>
>> try
>> {
>> - frame_debug_printf ("trying unwinder \"%s\"", unwinder->name);
>> + frame_debug_printf ("trying unwinder \"%s\"", unwinder->name ());
>> res = unwinder->sniffer (unwinder, this_frame, this_cache);
>> }
>> catch (const gdb_exception &ex)
>> @@ -335,6 +335,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
>> +{
>> + return stop_reason_p (this_frame, this_prologue_cache);
>> +}
> If you change this method to call the base implementation if
> stop_reason_p is nullptr, then the various instances of
> frame_unwind_legacy can just pass nullptr instead of explicitly setting
> it in their declarations. i.e:
>
> /* 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
> {
> if (stop_reason_p == nullptr)
> return frame_unwind::stop_reason (this_frame, this_prologue_cache);
> return stop_reason_p (this_frame, this_prologue_cache);
> }
>
> Also, not for this patch but a separate one (if you agree): with the
> change above, it's possible to move the stop_reason argument in the
> frame_unwind_legacy constructor to a later position to make it optional,
> thus simplifying a bit the declaration of most frame_unwind_legacy
> instances. I have a patch in my branch to do that, if you're interested.
This makes sense. I wanted to minimize review difficulty with these, but
it definitely feels like a good idea for the future work of moving
unwinders from legacy into their own classes.
>
>> +/* This method just passes the parameters to the callback pointer. */
>> +int
>> +frame_unwind_legacy::sniffer (const struct frame_unwind *self,
>> + const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const
>> +{
>> + return sniffer_p (self, this_frame, this_prologue_cache);
>> +}
> Many frame_unwind_legacy instances use default_frame_sniffer, which
> simply returns 1. Since this patch already defines an equivalent
> frame_unwind::sniffer (), it's possible to remove the need of specifying
> default_frame_sniffer in the declarations of many
> frame_uwnind_legacy instances if this method is made to call the parent
> class' implementation if sniffer_p is nullptr:
>
> /* This method just passes the parameters to the callback pointer. */
> int
> frame_unwind_legacy::sniffer (const frame_info_ptr &this_frame,
> void **this_prologue_cache) const
> {
> if (sniffer_p == nullptr)
> return frame_unwind::sniffer (this_frame, this_prologue_cache);
> return sniffer_p (this, this_frame, this_prologue_cache);
> }
Same as above, I agree that this is a great change but I worried it
would make for a harder review.
>
>> +/* This method just passes the parameters to the callback pointer. */
>> +struct gdbarch *
>> +frame_unwind_legacy::prev_arch (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const
>> +{
>> + if (prev_arch_p == nullptr)
>> + error (_("No prev_arch callback installed"));
>> + return prev_arch_p (this_frame, this_prologue_cache);
>> +}
> Using the prev_arch () method is a bit awkward, both before your patch
> and after. The problem is that frame_unwind_arch () — the only caller of
> this method — has to check whether the unwinder implements prev_arch and
> if not, call the get_frame_arch () function.
>
> It's possible to simplify this by adding a default implementation of
> prev_arch () in frame_unwind which calls get_frame_arch (), and invoke
> it here if prev_arch_p == nullptr:
>
> /* This method just passes the parameters to the callback pointer. */
> struct gdbarch *
> frame_unwind_legacy::prev_arch (const frame_info_ptr &this_frame,
> void **this_prologue_cache) const
> {
> if (prev_arch_p == nullptr)
> return frame_unwind::prev_arch (this_frame, this_prologue_cache);
> return prev_arch_p (this_frame, this_prologue_cache);
> }
>
> Then, in frame_unwind_arch () we can just call the unwinder's
> prev_arch () method without worry.
>
>> diff --git a/gdb/frame-unwind.h b/gdb/frame-unwind.h
>> index deab4f7dbfb..62da34b6ee8 100644
>> --- a/gdb/frame-unwind.h
>> +++ b/gdb/frame-unwind.h
>> @@ -169,25 +169,153 @@ 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;
>> +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;
>> + }
>> +
>> + enum frame_type type () const
>> + {
>> + return m_type;
>> + }
>> +
>> + enum frame_unwind_class unwinder_class () const
>> + {
>> + 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. */
>> + 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);
>> + }
> Instead of calling default_frame_unwind_stop_reason () from here, I
> think it's better to convert default_frame_unwind_stop_reason () into
> frame_unwind::stop_reason ().
>
>> +
>> + /* Default frame sniffer. Will always return that the unwinder
>> + is able to unwind the frame. */
>> + virtual int sniffer (const frame_unwind *self,
>> + const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const
>> + {
>> + return 1;
>> + }
> Similar idea here. This method is identical to default_frame_sniffer (),
> so I suggest converting the latter into frame_unwind::sniffer ().
>
> Also, the 'self' argument is redundant with the implicit 'this' argument
> and should be removed.
>
>> + /* 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. */
> Rather than throwing an error at runtime to check if an unwinder has
> implemented the desired functionality, I believe it's better to do the
> check at compile time by making these methods pure virtual.
>
> This can be done to this_id () and prev_register (). prev_arch () can be
> turned into a useful default implementation by making it call
> get_frame_arch () as I mentioned before.
>
> This leaves just dealloc_cache (), so this comment can be moved to it.
I remember trying to make the methods pure-virtual and getting linker
errors when testing locally.
I know that after that I found one unwinder that didn't set one of the
pointers, so maybe that was fixed... If you managed to build with
--enable-targets=all, I think we're safe.
>
>> +
>> + /* 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);
>> + }
> So this becomes:
>
> /* 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;
>
>
>> + /* 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
>> + {
>> + error (_("No method prev_register implemented for unwinder %s"), m_name);
>> + }
> This becomes:
>
> /* 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;
>
>> + /* Properly deallocate the cache. */
>> + virtual void dealloc_cache (frame_info *self, void *this_cache) const
>> + {
>> + error (_("No method dealloc_cache implemented for unwinder %s"), m_name);
>> + }
> IIUC this error is an implementation problem in GDB, not an error
> condition that the user can understand or do anything about. I suggest
> changing to internal_error ().
That's fair. The prev_frame implementation needed error(), and I went
for consistency. With pure virtual method and the prev_arch changes I
think this makes more sense!
>
>> + /* 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);
>> + }
> As mentioned earlier, this becomes:
>
> /* Get the previous architecture. */
> virtual struct gdbarch *prev_arch (const frame_info_ptr &this_frame,
> void **this_prologue_cache) const
> {
> return get_frame_arch (this_frame);
> }
>
>
>> +};
>> +
>> +/* 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;
>> +public:
>> + frame_unwind_legacy (const char *n, frame_type t, frame_unwind_class c,
>> + frame_unwind_stop_reason_ftype *sr,
> As mentioned before, if this argument is moved later, it can be made
> optional. Probably best done in a separate patch, if you agree with the
> change.
Makes sense
>
>> + frame_this_id_ftype *ti,
>> + frame_prev_register_ftype *pr,
>> + const struct frame_data *ud,
> It turns out that no frame_unwind_legacy instance passes a frame_data
> object, so this argument can simply be removed and nullptr can be passed
> to the parent class' constructor. Not sure if this is better done as a
> separate patch, or as part of this patch.
Oh, thanks for noticing that!
I think it's better to have it separate so we minimize things that can
go wrong in one single change...
>
>> + frame_sniffer_ftype *s,
> With my earlier suggestion, you can make this argument optional:
>
> frame_sniffer_ftype *s = nullptr,
>
>> + 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) { }
>> @@ -3072,11 +3070,15 @@ frame_unwind_arch (const frame_info_ptr &next_frame)
>> if (next_frame->unwind == NULL)
>> frame_unwind_find_by_frame (next_frame, &next_frame->prologue_cache);
>>
>> - if (next_frame->unwind->prev_arch != NULL)
>> - arch = next_frame->unwind->prev_arch (next_frame,
>> - &next_frame->prologue_cache);
>> - else
>> - arch = get_frame_arch (next_frame);
>> + try
>> + {
>> + arch = next_frame->unwind->prev_arch (next_frame,
>> + &next_frame->prologue_cache);
>> + }
>> + catch (gdb_exception &e)
>> + {
>> + arch = get_frame_arch (next_frame);
>> + }
> With the changes I suggested earlier, this becomes simply:
>
> arch = next_frame->unwind->prev_arch (next_frame,
> &next_frame->prologue_cache);
>
>> next_frame->prev_arch.arch = arch;
>> next_frame->prev_arch.p = true;
>> diff --git a/gdb/frv-linux-tdep.c b/gdb/frv-linux-tdep.c
>> index 7a7c4904475..4941e4a0741 100644
>> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
>> index be865988a2f..82d74692893 100644
>> --- a/gdb/python/py-unwind.c
>> +++ b/gdb/python/py-unwind.c
>> @@ -826,7 +826,7 @@ pyuw_sniffer (const struct frame_unwind *self, const frame_info_ptr &this_frame,
>> {
>> PYUW_SCOPED_DEBUG_ENTER_EXIT;
>>
>> - struct gdbarch *gdbarch = (struct gdbarch *) (self->unwind_data);
>> + struct gdbarch *gdbarch = (struct gdbarch *) (self->unwind_data ());
>> cached_frame_info *cached_frame;
>>
>> gdbpy_enter enter_py (gdbarch);
>> @@ -967,6 +967,41 @@ struct pyuw_gdbarch_data_type
>>
>> static const registry<gdbarch>::key<pyuw_gdbarch_data_type> pyuw_gdbarch_data;
>>
>> +/* Class for frame unwinders registered by the Python architecture callback. */
>> +class frame_unwind_python : public frame_unwind
>> +{
>> +public:
>> + frame_unwind_python (const struct frame_data *newarch)
>> + : frame_unwind ("python", NORMAL_FRAME, FRAME_UNWIND_EXTENSION, newarch)
>> + { }
>> +
>> + /* No need to override stop_reason, we want the default. */
>> +
>> + int sniffer (const frame_unwind *self, const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const override
>> + {
>> + return pyuw_sniffer (self, this_frame, this_prologue_cache);
>> + }
>> +
>> + void this_id (const frame_info_ptr &this_frame, void **this_prologue_cache,
>> + struct frame_id *id) const override
>> + {
>> + pyuw_this_id (this_frame, this_prologue_cache, id);
>> + }
>> +
>> + struct value *prev_register (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache,
>> + int regnum) const override
>> + {
>> + return pyuw_prev_register (this_frame, this_prologue_cache, regnum);
>> + }
>> +
>> + void dealloc_cache (frame_info *self, void *this_cache) const override
>> + {
>> + pyuw_dealloc_cache (self, this_cache);
>> + }
>> +};
> For all the methods above, I think the functions should be converted to
> methods rather than just having the methods call them. i.e.:
>
> pyuw_sniffer () → frame_unwind_python::sniffer ()
> pyuw_this_id → frame_unwind_python::this_id ()
> pyuw_prev_register () → frame_unwind_python::prev_register ()
> pyuw_dealloc_cache () → frame_unwind_python::dealloc_cache ()
yeah, makes sense.
>
>> diff --git a/gdb/tramp-frame.c b/gdb/tramp-frame.c
>> index 6368f67a2e7..94ca85df007 100644
>> --- a/gdb/tramp-frame.c
>> +++ b/gdb/tramp-frame.c
>> @@ -124,7 +124,7 @@ tramp_frame_sniffer (const struct frame_unwind *self,
>> const frame_info_ptr &this_frame,
>> void **this_cache)
>> {
>> - const struct tramp_frame *tramp = self->unwind_data->tramp_frame;
>> + const struct tramp_frame *tramp = self->unwind_data ()->tramp_frame;
>> CORE_ADDR pc = get_frame_pc (this_frame);
>> CORE_ADDR func;
>> struct tramp_frame_cache *tramp_cache;
>> @@ -143,6 +143,48 @@ tramp_frame_sniffer (const struct frame_unwind *self,
>> return 1;
>> }
>>
>> +class frame_unwind_trampoline : public frame_unwind
>> +{
>> +private:
>> + frame_prev_arch_ftype *prev_arch_p;
>> +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)
>> + { }
>> +
>> + int sniffer(const frame_unwind *self, const frame_info_ptr &this_frame,
>> + void **this_prologue_cache) const override
>> + {
>> + return tramp_frame_sniffer (self, this_frame, this_prologue_cache);
>> + }
>> + void this_id (const frame_info_ptr &this_frame, void **this_prologue_cache,
>> + struct frame_id *id) const override
>> + {
>> + tramp_frame_this_id (this_frame, this_prologue_cache, id);
>> + }
>> + struct value *prev_register (const frame_info_ptr &this_frame,
>> + void **this_prologue_cache,
>> + int regnum) const override
>> + {
>> + return tramp_frame_prev_register (this_frame, this_prologue_cache, regnum);
>> + }
>> +
>> + 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
>> + { }
>> +};
> Same comment as in frame_unwind_python: for all the methods above, I
> think the functions should be converted to methods rather than just
> having the methods call them.
>
Thanks again for the thorough review. I'll check your repo and
incorporate many of the suggestions, re-test everything and send a v5
with it!
--
Cheers,
Guinevere Larsen
She/Her/Hers
More information about the Gdb-patches
mailing list