Re: add set cp-abi command

[Jim Ingham <jingham at apple dot com>]

Daniel,

> 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.

Okay, this was the standard trope in Tcl-land for what to do with an array
that you really didn't think was going to change in size, but just in case,
so I didn't think it was so complicated...

In fact, since the ABI's are all added in _initialize functions, it would be
even simpler just to fix the size of the array.  When somebody adds the
"last straw" ABI to gdb, we can just spit out a message saying increase this
number, and that developer can do so and recompile.  This is safe since I
don't think we really have the intention that we are adding these things
dynamically at runtime...

> 
>> ! 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.

Yeah, I particularly like the bit where we initialize the mi twice, just to
make sure it ends up at the end of the list!

> 
>> ! 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.

I vote for just fixing the size, then.  The ABIs are really fixed at build
time, and reallocing like in the original strikes me as ugly.

> 
>> + 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?
> 

Nah, cutting from too many windows at a time.  The patch below is right.

>> +   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.
> 

OK.

>> +   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.
> 

I don't want to have the auto abi be a special beast off to one side.  This
seems to add to the complexity for no apparent reason.

We can more simply address your real objection by copying the whole
structure, then putting the strings back in place...  See the patch below.


>> !   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.

I disagree, it adds complexity to the actual code for the sake of
maintaining a few consts.  I don't think this is a good tradeoff.

> 
>>             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 :)

Way too much work to get there, however.  Another point that is unclear to
me, should this instead be:

               if (cp_abi_is_auto_p ())
                   set_cp_abi_as_auto_default ("gnu_v3");
                 break;

I would have to change set_... to take the name rather than a structure, and
look it up (and add a lookup_cp_abi to be nice).  But it fits with the set
lang more.  OTOH, I have already spent more time on this that it deserves by
a long shot, so...

I am including a new patch, with just the cp-abi.c bits to keep it smaller,
and I didn't edit this one to change the indentation goofs that the mailer
added (I did with the last patch, oh fun...).

Jim

Index: cp-abi.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-abi.c,v
retrieving revision 1.3
diff -c -w -p -r1.3 cp-abi.c
*** cp-abi.c    2002/01/04 18:20:19     1.3
--- cp-abi.c    2002/03/18 19:38:55
***************
*** 21,33 ****
  #include "defs.h"
  #include "value.h"
  #include "cp-abi.h"
  
! struct cp_abi_ops current_cp_abi;
  
! struct cp_abi_ops *cp_abis;
  
! int num_cp_abis = 0;
  
  enum ctor_kinds
  is_constructor_name (const char *name)
  {
--- 21,39 ----
  #include "defs.h"
  #include "value.h"
  #include "cp-abi.h"
+ #include "command.h"
+ #include "ui-out.h"
+ #include "gdbcmd.h"
  
! static struct cp_abi_ops current_cp_abi = {"", NULL};
! static struct cp_abi_ops auto_cp_abi = {"auto", NULL};
  
! #define CP_ABI_MAX 8
  
! static struct cp_abi_ops *cp_abis[CP_ABI_MAX];
  
+ static int num_cp_abis = 0;
+ 
  enum ctor_kinds
  is_constructor_name (const char *name)
  {
*************** value_rtti_type (struct value *v, int *f
*** 87,109 ****
  }
  
  int
! register_cp_abi (struct cp_abi_ops abi)
  {
!   cp_abis =
!     xrealloc (cp_abis, (num_cp_abis + 1) * sizeof (struct cp_abi_ops));
    cp_abis[num_cp_abis++] = abi;
  
    return 1;
  
  }
  
  int
  switch_to_cp_abi (const char *short_name)
  {
    int i;
    for (i = 0; i < num_cp_abis; i++)
!     if (strcmp (cp_abis[i].shortname, short_name) == 0)
!       current_cp_abi = cp_abis[i];
    return 1;
  }
  
--- 93,217 ----
  }
  
  int
! register_cp_abi (struct cp_abi_ops *abi)
! {
!   if (num_cp_abis == CP_ABI_MAX)
      {
!       error ("Too many CP ABI's, please increase CP_ABI_MAX in cp-abi.c");
!     }
!   
    cp_abis[num_cp_abis++] = abi;
  
    return 1;
  
  }
  
+ void
+ set_cp_abi_as_auto_default (struct cp_abi_ops *abi)
+ {
+ 
+   if (auto_cp_abi.longname != NULL)
+     xfree (auto_cp_abi.longname);
+ 
+   if (auto_cp_abi.doc != NULL)
+     xfree (auto_cp_abi.doc);
+ 
+   auto_cp_abi = *abi;
+ 
+   auto_cp_abi.shortname = "auto";
+   auto_cp_abi.longname = (char *) xmalloc (strlen ("currently ")
+                                          + strlen (abi->shortname));
+   sprintf (auto_cp_abi.longname, "currently %s",
+          abi->shortname);
+   
+   auto_cp_abi.doc = (char *) xmalloc (strlen ("currently ")
+                                     + strlen (abi->shortname));
+   sprintf (auto_cp_abi.doc, "currently %s",
+          abi->shortname);
+   
+   /* 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. */
+                  
+   if (cp_abi_is_auto_p ())
+     switch_to_cp_abi ("auto");
+ }
+ 
+ int
+ cp_abi_is_auto_p ()
+ {
+   if (strcmp (current_cp_abi.shortname, "auto") == 0)
+     return 1;
+   else
+     return 0;
+ }
+ 
  int
  switch_to_cp_abi (const char *short_name)
  {
    int i;
+ 
    for (i = 0; i < num_cp_abis; i++)
!     if (strcmp (cp_abis[i]->shortname, short_name) == 0)
!       {
!       current_cp_abi = *cp_abis[i];
        return 1;
        }
  
+   return 0;
+ }
+ 
+ void
+ show_cp_abis (int from_tty)
+ {
+   int i;
+   ui_out_text (uiout, "The available C++ ABIs are:\n");
+  
+   ui_out_tuple_begin (uiout, "cp-abi-list");
+   for (i = 0; i < num_cp_abis; i++)
+     {
+       ui_out_field_string (uiout, "cp-abi", cp_abis[i]->shortname);
+       ui_out_text_fmt (uiout, " - %s\n", cp_abis[i]->doc);
+     }
+   ui_out_tuple_end (uiout);
+ 
+ }
+ 
+ void
+ set_cp_abi_cmd (char *args, int from_tty)
+ {
+ 
+   if (args == NULL)
+     {
+       show_cp_abis (from_tty);
+       return;
+     }
+ 
+   if (!switch_to_cp_abi (args))
+     error ("Could not find ABI: \"%s\" in ABI list\n", args);
+ }
+ 
+ void
+ show_cp_abi_cmd (char *args, int from_tty)
+ {
+   ui_out_text (uiout, "The currently selected C++ abi is: ");
+  
+   ui_out_field_string (uiout, "cp-abi", current_cp_abi.shortname);
+   ui_out_text (uiout, ".\n");
+ }
+ 
+ void
+ _initialize_cp_abi (void)
+ {
+   struct cmd_list_element *cmd;
+ 
+   register_cp_abi (&auto_cp_abi);
+   switch_to_cp_abi ("auto");
+ 
+   cmd = add_cmd ("cp-abi", class_obscure , set_cp_abi_cmd,
+                "Set the ABI used for inspecting C++ objects.\n\
+ \"set cp-abi\" with no arguments will list the available ABIs.",
&setlist);
+ 
+   cmd = add_cmd ("cp-abi", class_obscure, show_cp_abi_cmd,
+                "Show the ABI used for inspecting C++ objects.",
&showlist);
+ 
+ }


-- 
++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=++=
Jim Ingham                              jingham@apple.com
Developer Tools - gdb