Re: PATCH: don't corrupt cv_type chain

Jim Blandy
2002-05-04T02:59:29Z

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?