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

[Guinevere Larsen <blarsen@redhat.com>]

On 08/03/2024 18:07, Tom Tromey wrote:
>>>>>> 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.
Sure, I can do this.
>
>> +/* 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.
Any specific reason why? If the problem is the single-letter parameters, 
I can change that (it was from an earlier iteration), but other than 
that, I'm not sure what you find weird in this.
>
>> +  /* 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?

not all classes have these methods defined, and if we make it pure 
virtual, all classes will need to implement them (or we get linker 
errors), so it seems easier to implement a base error in here.

>
>> +  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.
according to cppreference, constexpr constructors must not have virtual 
base classes, so unfortunately it isn't possible =/
>
> 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.

I tried compiling without this patch to see what's GDB startup time, 
then checked the startup time with this patch. In my system, I can't see 
a difference in startup time with the "time" command (so 0.01s precision).

I could try to find a way to delay instantiation to when the unwinder is 
installed, so we avoid unnecessary constructor calls.

>> +	  = 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.
I am only allocating things on the obstack because I couldn't figure out 
a better solution to dynamically allocate the unwinder in a way that 
wouldn't leak the memory if gdbarch was de-allocated. If you are dead 
set on delaying constructing the objects at startup, I would probably 
make gdbarch store unique_ptrs instead of raw pointers, or some other 
similar thing.
>
> Also a missing space.
>
> Tom
>

-- 
Cheers,
Guinevere Larsen
She/Her/Hers