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

[Guinevere Larsen <blarsen@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3770 bytes --]

I mentioned as a reply to the cover letter that this patch series had a 
crash in arm32 bit. This patch had the problem and a quick fix I inlined.

Trimming the diff for readability.

On 4/8/24 17:19, Guinevere Larsen wrote:
> diff --git a/gdb/tramp-frame.c b/gdb/tramp-frame.c
> index 2fcc3814e59..7fee4d5c71d 100644
> --- a/gdb/tramp-frame.c
> +++ b/gdb/tramp-frame.c
> @@ -123,7 +123,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;
> @@ -142,6 +142,46 @@ 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
> +  {

This place should have:

     if (prev_arch_p == nullptr)
       error (_("No prev_arch callback installed"));


I've fixed this locally, but since this patch is over 100Kb, I would 
prefer to not spam the list :)

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

> +    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
> +  { }
> +};
> +
>   void
>   tramp_frame_prepend_unwinder (struct gdbarch *gdbarch,
>   			      const struct tramp_frame *tramp_frame)
> @@ -160,16 +200,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);
>     frame_unwind_prepend_unwinder (gdbarch, unwinder);
>   }