From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11446 invoked by alias); 4 May 2002 00:32:30 -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 11438 invoked from network); 4 May 2002 00:32:29 -0000 Received: from unknown (HELO nevyn.them.org) (128.2.145.6) by sources.redhat.com with SMTP; 4 May 2002 00:32:29 -0000 Received: from drow by nevyn.them.org with local (Exim 3.35 #1 (Debian)) id 173nTR-0001Nv-00 for ; Fri, 03 May 2002 20:32:29 -0400 Date: Fri, 03 May 2002 17:32:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sources.redhat.com Subject: Re: PATCH: don't corrupt cv_type chain Message-ID: <20020504003229.GB4915@nevyn.them.org> Mail-Followup-To: gdb-patches@sources.redhat.com References: <20020504002026.6E6BA5EA11@zwingli.cygnus.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20020504002026.6E6BA5EA11@zwingli.cygnus.com> User-Agent: Mutt/1.5.1i X-SW-Source: 2002-05/txt/msg00090.txt.bz2 On Fri, May 03, 2002 at 07:20:26PM -0500, Jim Blandy wrote: > > This fixes the second failure in hang.exp. > > 2002-05-03 Jim Blandy > > * stabsread.c (cleanup_undefined_types): Use replace_type, not memcpy. This bit is definitely correct. Sorry I didn't notice this when I caught the other instance of the problem and added replace_type. However, I do not agree with your comment changes. More below. > (read_type): Doc fix. > * gdbtypes.c (replace_type): Doc fix. > > Index: gdb/gdbtypes.c > =================================================================== > RCS file: /cvs/src/src/gdb/gdbtypes.c,v > retrieving revision 1.44 > diff -c -r1.44 gdbtypes.c > *** gdb/gdbtypes.c 26 Apr 2002 20:08:18 -0000 1.44 > --- gdb/gdbtypes.c 4 May 2002 00:18:05 -0000 > *************** > *** 521,530 **** > > /* Replace the contents of ntype with the type *type. > > ! This function should not be necessary, but is due to quirks in the stabs > ! reader. This should go away. It does not handle the replacement type > ! being cv-qualified; it could be easily fixed to, but it should go away, > ! remember? */ > void > replace_type (struct type *ntype, struct type *type) > { > --- 521,530 ---- > > /* Replace the contents of ntype with the type *type. > > ! When building recursive types, it is necessary to update a type's > ! definition after people already have references to it. The C > ! language's concept of an `incomplete type' is an acknowledgement of > ! this. */ > void > replace_type (struct type *ntype, struct type *type) > { First, you removed the editorial; I think it is correct still. More on that at the bottom. Second, you removed the fact that it will not properly handle the replacement type being cv-qualified. That's important! > Index: gdb/stabsread.c > =================================================================== > RCS file: /cvs/src/src/gdb/stabsread.c,v > retrieving revision 1.31 > diff -c -r1.31 stabsread.c > *** gdb/stabsread.c 4 May 2002 00:02:50 -0000 1.31 > --- gdb/stabsread.c 4 May 2002 00:18:08 -0000 > *************** > *** 2537,2543 **** > the related problems with unnecessarily stubbed types; > someone motivated should attempt to clean up the issue > here as well. Once a type pointed to has been created it > ! should not be modified. */ > replace_type (type, xtype); > TYPE_NAME (type) = NULL; > TYPE_TAG_NAME (type) = NULL; > --- 2537,2560 ---- > the related problems with unnecessarily stubbed types; > someone motivated should attempt to clean up the issue > here as well. Once a type pointed to has been created it > ! should not be modified. > ! > ! Well, it's not *absolutely* wrong. Constructing recursive > ! types (trees, linked lists) necessarily entails modifying > ! types after creating them. Constructing any loop structure > ! entails side effects. The Dwarf 2 reader does handle this > ! more gracefully (it never constructs more than once > ! instance of a type object, so it doesn't have to copy type > ! objects wholesale), but it still mutates type objects after > ! other folks have references to them. > ! > ! Keep in mind that this circularity/mutation issue shows up > ! at the source language level, too: C's "incomplete types", > ! for example. So the proper cleanup, I think, would be to > ! limit GDB's type smashing to match exactly those required > ! by the source language. So GDB could have a > ! "complete_this_type" function, but never create unnecessary > ! copies of a type otherwise. */ > replace_type (type, xtype); > TYPE_NAME (type) = NULL; > TYPE_TAG_NAME (type) = NULL; DWARF-2 has to mutate types somewhat, certainly; but this sort of smashing is avoidable and quite disgusting. There should be an explicit list of things it is safe to modify, rather than the axe that is replace_type. Adding a size or a a field list is legitimate, but many of the other fields should not change. I will try to separate the qualifiers and core type information, as I promised to do when I filed gdb/277, in the next week or two. After that, the information in this 'common' area (common across cv-/as- qualification, for instance) may be updated but the rest of the type should be left alone. -- Daniel Jacobowitz Carnegie Mellon University MontaVista Software Debian GNU/Linux Developer