From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23826 invoked by alias); 16 Mar 2002 03:41:11 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 23757 invoked from network); 16 Mar 2002 03:41:10 -0000 Received: from unknown (HELO nevyn.them.org) (128.2.145.6) by sources.redhat.com with SMTP; 16 Mar 2002 03:41:10 -0000 Received: from drow by nevyn.them.org with local (Exim 3.35 #1 (Debian)) id 16m54L-0001AC-00; Fri, 15 Mar 2002 22:41:21 -0500 Date: Fri, 15 Mar 2002 19:41:00 -0000 From: Daniel Jacobowitz To: Jim Ingham Cc: gdb-patches@sources.redhat.com Subject: Re: add set cp-abi command Message-ID: <20020315224121.B3612@nevyn.them.org> Mail-Followup-To: Jim Ingham , gdb-patches@sources.redhat.com References: <20020314162627.A9503@nevyn.them.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.3.23i X-SW-Source: 2002-03/txt/msg00255.txt.bz2 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