Re: add set cp-abi command

[Daniel Jacobowitz <drow at mvista dot com>]

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