Re: Infinite loop in make_cv_type

[Daniel Jacobowitz <drow@mvista.com>]

On Fri, Feb 22, 2002 at 11:40:38AM +0000, Richard Earnshaw wrote:
> While testing cplusfuncs.exp on ARM/NetBSD (a.out) with gcc-3 current, gdb 
> is getting stuck in an infinite loop in gdbtypes.c:make_cv_type and I'm 
> trying to work out what this is supposed to do.  The scenario I'm seeing 
> is that the type ring has become corrupted as follows along the 
> TYPE_CV_TYPE chain
> 
>           type
> 	    |
>             V
> 	  var1<----+
>             |      |
>             +------+
> 
> Given that this is supposed to be a loop, it's clearly bogus.

Definitely.

Just checking - CVS head GDB?  I fixed a problem with identical
symptoms in the stabs reader about three weeks ago.

2002-02-03  Daniel Jacobowitz  <drow@mvista.com>

        PR gdb/280
        * gdbtypes.c (replace_type): New function.
        * gdbtypes.h (replace_type): Add prototype.
        * stabsread.c (read_type): Use replace_type.

> The reason for this seems to be that dbx_lookup_type is returning the 
> address of var1 as the place to put the modified type; ie it's asking 
> make_cv_type to modify an existing type variant.  I'm not entirely sure 
> that this is correct, but it may be that this is how the stabs reader 
> creates a type -- ie it creates it, and then modifies it as it reads in 
> more attributes.

No, that's not right.  Once the type is created cv-qualifiers should
never be added to it, unless you've got some really really bogus stabs. 
Could you supply the stabs that were being parsed when gdb hung?

> There are several issues with make_cv_type:
> 
> 1) Why is the top loop not executed at all when the cv loop has no 
> variants?  It could be that we want the base type to be returned, and we 
> never can as the code is written (we end up creating an identical copy).

We don't want the base type to be returned from any of the current
callers, so I imagine no one fixed that.

Er, there seems to be an exception in check_typedef for stubbed/opaque
types.  Ew.

> 2) The chain updating at the end of the function is written in a bizarre 
> way, in that it assumes we will be inserting in the last entry before the 
> base type.  While this has so-far been the case (because of the way the 
> top loop was written), it doesn't seem like the normal way to express such 
> an insertion operation (it implies that we could be dropping part of the 
> chain).

True.  This could possibly cause your symptom, too.

> 3) There's no support for updating an existing entry in the loop.
> 
> Adding the patch below solves that problem, but we then segfaults on 
> another type because the TYPE_CV_TYPE loop has been smashed by the memset 
> in make_pointer_type.  This appears to happen at several places in that 
> file, and it seems to me that we really need a function realloc_type() 
> that is analogous to alloc_type, but recycles the type in a sensible 
> manner.  I've written such a function as well.
> 
> Of course, it could be that the stabs reader is doing something completely 
> bogus by passing the addresses of existing types into the gdbtypes code, 
> in which case that will have to be rewritten to prevent this; but it 
> doesn't seem like that was the intention.

While 1) and 2) should be fixed, 3) should not be.  See the comment I
added to stabsread on February 2nd:

            /* This is the absolute wrong way to construct types. Every
               other debug format has found a way around this problem and
               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;

Before I added that, it said:
	    *type = *xtype;
which obviously destroyed the CV chains.


I'm going to smash make_cv_type sometime soon, by the way.  See
gdb/277.

> <date>  Richard Earnshaw  (rearnsha@arm.com)
> 
> 	* gdbtypes.c (make_cv_type): Handle being asked to modify an 
> 	existing type in the chain.

I'd rather that we detect this case and abort; and that we never hit
the abort :)

> 	(realloc_type): Cleanly recyle memory for a type.
> 	(make_pointer_type): Use realloc_type to recycle an existing type.
> 	(make_reference_type): Likewise.
> 	(make_function_type): Likewise.
> 	(smash_to_member_type): Likewise.
> 	(smash_to_method_type): Likewise.

The other places are quite probably correct.  There's still an issue of
some subtlety of what to do with the existing cv-chain; we don't want
it getting lost.  Rather, we want to update all of it.  Thus gdb/277.

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