Re: add set cp-abi command

Daniel Jacobowitz
2002-03-16T03:41:11Z

On Fri, Mar 15, 2002 at 03:52:47PM -0800, Jim Ingham wrote:
> Daniel,
>
> Okay, how about this.
>
> I changed the set/show as per set language.
>
> I added an "auto" abi. This ended up being a little tricky because I
> don't know the order in which the initializers get run, but this works.
>
> In the process of doing this I got really annoyed that EVERYTHING was
> done by copying structures around. If we think that it is really
> important to have the current_cp_abi be a structure rather than a
> pointer to a structure so there was one less level of indirection,
> that's fine. But there is no reason to manage the internal list this
> way as well. So I changed cp_abis to be an array of pointers to
> cp_abi_ops structures, and had to change the auto-grow mechanism to
> account for it.
>
> I also took out the extern def'ns of num_cp_abis, cp_abis and
> current_cp_abi out of cp-abi.h. They weren't currently used outside the
> module, and if we end up needing to use them, we should provide
> interfaces, rather than just poking around at the data.
>
> I haven't written a doc note for this, I have run out of time for this
> today, but thought I would give you a look before the day is out...

Better. I have a couple of nits, but mostly I like it. You've
probably thought of some of these; I realize it's a WIP.

I don't think you had to do anything nearly as complicated to the array
growth code, for one thing. You could have simply grown it by sizeof
(struct cp_abi_ops *) instead of sizeof (struct cp_abi_ops) as it was
before.

> ! static struct cp_abi_ops current_cp_abi;

As in your other message... we can not assume anything about
initializer order. This periodically makes me cry.

> ! static struct cp_abi_ops auto_cp_abi = {"auto", NULL};
>
> ! #define INITIAL_CP_ABI_MAX 8
>
> ! static struct cp_abi_ops *orig_cp_abis[INITIAL_CP_ABI_MAX];
> ! static struct cp_abi_ops **cp_abis = orig_cp_abis;
> ! static int max_cp_abis = INITIAL_CP_ABI_MAX;

> int
> ! register_cp_abi (struct cp_abi_ops *abi)
> ! {
> ! if (num_cp_abis == max_cp_abis)
> {
> ! struct cp_abi_ops **new_abi_list;
> ! int i;
> !
> ! max_cp_abis *= 2;
> ! new_abi_list = (struct cp_abi_ops **) xmalloc (max_cp_abis *
> sizeof (struct cp_abi_ops *));
> ! for (i = 0; i < num_cp_abis; i++)
> ! new_abi_list[i] = cp_abis[i];
> !
> ! if (cp_abis != orig_cp_abis)
> ! xfree (cp_abis);
> !
> ! cp_abis = new_abi_list;
> ! }
> !
> cp_abis[num_cp_abis++] = abi;
>
> return 1;
>
> }

No point in having all this complexity. We could dynamically allocate
the array in the first place, or even use a linked list if that were
simpler. I think an array of just pointers, managed the same way the
old array was, is fine. A couple extra xreallocs are worth the
simplicity.

> + void
> + set_cp_abi_as_auto_default (struct cp_abi_ops *abi)
> + {
> +
> + if (auto_cp_abi.longname != NULL)
> + xfree (auto_cp_abi.longname);

Crash the first time you set an ABI, I think - auto_cp_abi.longname
points to "auto" in your rodata. Did this survive on Darwin?

> + auto_cp_abi.longname = (char *) xmalloc (11 + strlen
> (abi->shortname));

11? Oh, I see, "currently \0". Please use a sizeof("blah") for this
sort of thing; magic numbers are bad.

> + auto_cp_abi.is_destructor_name = abi->is_destructor_name;
> + auto_cp_abi.is_constructor_name = abi->is_constructor_name;
> + auto_cp_abi.is_vtable_name = abi->is_vtable_name;
> + auto_cp_abi.is_operator_name = abi->is_operator_name;
> + auto_cp_abi.virtual_fn_field = abi->virtual_fn_field;
> + auto_cp_abi.rtti_type = abi->rtti_type;
> + auto_cp_abi.baseclass_offset = abi->baseclass_offset;
> +
> + /* Since we copy the current ABI into current_cp_abi instead of using
> + a pointer, if auto is currently the default, we need to reset
> it. */

Is there some way we could manage all this with pointers instead? I
don't like having to copy this structure above. It's just another bug
every time I add an ABI-specific method, of which I suspect I'll need a
few more.

> ! char *longname; /* These two can't be const, because I need to */
> ! char *doc; /* change the name for the auto abi. */

Then perhaps the name of the currently selected auto ABI should be
stored somewhere else... that should be simpler.

> const char *name = SYMBOL_NAME (&objfile->msymbols[i]);
> if (name[0] == '_' && name[1] == 'Z')
> {
> + if (cp_abi_is_auto_p ())
> switch_to_cp_abi ("gnu-v3");
> break;
> }

This bit, of course, is great :)

--
Daniel Jacobowitz Carnegie Mellon University
MontaVista Software Debian GNU/Linux Developer