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

Guinevere Larsen
2024-10-18T17:40:44Z

On 10/17/24 7:53 PM, Tom Tromey wrote:
> Guinevere> I am not a big fan of the registry subsystem, so I would prefer to
> Guinevere> keep it as a simple vector. However, if you are concerned about the
> Guinevere> opaqueness change, Tromey, I can try to make it more opaque. I just
> Guinevere> personally find that registry tends to make the code too opaque for
> Guinevere> programmers as well as the program itself :-)
>
> I think gdbarch is exposed to a lot of tdep code, and this code in
> particular to do questionable things. So, preserving some opacity makes
> sense to me.
Understandable. It didn't sound like you were that worried about it
previously, apologies for seemingly ignoring this feedback
>
> Guinevere> So if you want more separation, I would personally prefer to look into
> Guinevere> only receiving a copy of the unwinder list, and gdbarch is responsible
> Guinevere> for adding and removing things. Just let me know and I can give it a
> Guinevere> shot
>
> I think it would be more worthwhile to understand your objections to the
> registry. It seems to me that switching back to using it is just a
> couple of lines of code, so it makes me think there must be some pretty
> strong reason.

My main objection is in the form of "this feels like the type of magic
you don't want on the project". It feels like some very complicated
arcane code that causes things to seem much more complicated for new
contributors. I understand it is important in some cases, but in here it
seemed to me like making a registry that stores a standard library class
was just obfuscating the code for little gain.

I would propose that instead, the getter for the vector returns a const
reference (asserts if it is called before initialization). gdbarch would
then provide a "initialize_unwinders" method and a "add_unwinder"
method, but those should only be called by frame-unwind.c (and that
would be explicitly called out in the comments of those functions). This
follows a bit more the traditional object orientation route for handling
opaqueness, I believe, which would make it slightly easier to have new
contributors up to speed.

I know this isn't the strongets, so I understand if you want me to keep
the registry stuff.

--
Cheers,
Guinevere Larsen
She/Her/Hers

> Tom
>