Re: [PATCH 1/4] gdb: make gdbarch store a vector of frame unwinders

[Guinevere Larsen <blarsen@redhat.com>]

On 08/03/2024 17:34, Tom Tromey wrote:
>>>>>> Guinevere Larsen <blarsen@redhat.com> writes:
>> Before this commit, all frame unwinders would be stored in the obstack
>> of a gdbarch and accessed by using the registry system. This made for
>> unwieldy code, and unnecessarily complex logic in the frame_unwinder
>> implementation, along with making frame_unwind structs be unable to have
>> non-trivial constructors.
>> Seeing as a future patch of this series wants to refactor the
>> frame_unwind struct to use inheritance, obstack storage would no longer
>> be viable. In preparation for that change, this commit adds an
>> std::vector to gdbarch to store the unwinders in.
>> There should be no user-visible changes.
> I'm not really sure about this patch.
>
> Like on the one hand, it is fine.  The arch is going to store the
> unwinder table.
>
> On the other hand, the registry system is there to let modules be kind
> of independent.  The lines are blurry though.
>
>> +std::vector<const frame_unwind*>&
> Missing spaces in here.
>
>> -static const registry<gdbarch>::key<struct frame_unwind_table>
>> -     frame_unwind_data;
> An alternative approach would be to just use a different type in here.
>
> This can use the default destruction approach and then it's just
> allocated with 'new'.  So then you can use any old C++ type.

I guess you're right, but would it make any real difference to have the 
gdbarch store a vector out right, or to store a vector in an obfuscated way?

I looked back through the git log and the latest change that could have 
added a significant change to how unwinders are stored could have 
happened in early 2014, so I don't really think the registry would 
meaningfully save in complexity.

But if you feel strongly that the registry is better, I can rework this 
patch to work with that instead... Or just drop it. Once Andrew informed 
of obstack_new, this patch felt unnecessary to the series as a whole, 
just a related cleanup that I forgot to reword.

>
> FWIW I think the real issue with obstack allocation isn't constructors
> but destruction.  See https://sourceware.org/pipermail/gdb-patches/2024-February/206888.html
I see what you mean (though I don't necessarily understand why). I would 
prefer if we could not restrict the destructors, but since GDB doesn't 
ever dealloc gdbarches to begin with, I don't think that would be a big 
issue either way.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers