Re: PATCH: don't corrupt cv_type chain

Daniel Jacobowitz
2002-05-04T00:32:30Z

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