PATCH: don't corrupt cv_type chain

PATCH: don't corrupt cv_type chain

[Jim Blandy]

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.
	(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)
  {
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;
***************
*** 5122,5131 ****
  			    && (TYPE_CODE (SYMBOL_TYPE (sym)) ==
  				TYPE_CODE (*type))
  			    && STREQ (SYMBOL_NAME (sym), typename))
! 			  {
! 			    memcpy (*type, SYMBOL_TYPE (sym),
! 				    sizeof (struct type));
! 			  }
  		      }
  		  }
  	      }
--- 5139,5145 ----
  			    && (TYPE_CODE (SYMBOL_TYPE (sym)) ==
  				TYPE_CODE (*type))
  			    && STREQ (SYMBOL_NAME (sym), typename))
!                           replace_type (*type, SYMBOL_TYPE (sym));
  		      }
  		  }
  	      }

Re: PATCH: don't corrupt cv_type chain

[Daniel Jacobowitz]

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

Re: PATCH: don't corrupt cv_type chain

[Jim Blandy]

Daniel Jacobowitz <drow@mvista.com> writes:
> > *** 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!

Yes, it is.  I'll put it back.


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

Isn't this, essentially, what my comment says?

Re: PATCH: don't corrupt cv_type chain

[Daniel Jacobowitz]

On Fri, May 03, 2002 at 09:59:26PM -0500, Jim Blandy wrote:
> Yes, it is.  I'll put it back.

Thanks!

> > 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.
> 
> Isn't this, essentially, what my comment says?

<reads it again>

Yes, it is.  Sorry for going off about it.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer