Re: PATCH: don't corrupt cv_type chain

[Daniel Jacobowitz <drow at mvista dot com>]

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  <jimb@redhat.com>
> 
> 	* 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