Re: Infinite loop in make_cv_type

[Daniel Jacobowitz <drow at mvista dot com>]

On Fri, Feb 22, 2002 at 06:21:42PM +0000, Richard Earnshaw wrote:
> > Just checking - CVS head GDB?  I fixed a problem with identical
> > symptoms in the stabs reader about three weeks ago.
> > 
> 
> Yep (cvs head).
> 
> > 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?
> 
> Hmm, it's something derived from this entry, though I'm not sure if that's 
> the entire string:
> 
> $27 = 0x509bc6 "__comp_dtor::813:_ZNSt14_STL_auto_lockD1Ev;2A.;operator=::8
> 14=#810,21,812,815=&816=k810,21;:_ZNSt14_STL_auto_lockaSERKS_;0A.;\\"
> 
> It's somewhere near that 816=k810. I suspect we are somehow parsing this 
> line twice (or part of it).

This makes sense.  The 'k' means const.  810 looks to be the number for
this type, since it's the first argument to operator=.  And the
demangling of the current method says it takes a const ref to its own
type (thus the ERKS_, if I remember my mangling rules correctly).

What compiler is this?  It doesn't look like GCC; I believe GCC always
generates type-pairs instead of type-nums.  Is this some ARM compiler
that obeys the v3 ABI?

You might want to grab the entire stab out of the object file with
objdump -G.

A testcase might help me sort through this a little better; I was the
last person to grub through the cv-type stuff.

> > > 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.
> 
> I thought so originally, but just fixing that didn't solve the problem.
> 
> > 
> > > 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.
> 
> 
> Urg.  It all seems a complete mess to me :-)  Another question: Given that 
> there seems to be a fundamental base type for the cv and as chains why are 
> they loops rather than chains with an additional pointer back to the base 
> type?  As things stand we potentially need to create a lattice if we have 
> multiple c-v variants and multiple address spaces (though we don't at 
> present - why not?).  That's a nightmare to maintain.

It is in fact a nightmare.  That's why I'm trying to eliminate most of
the chaining.  Using a base type structure and a type_variant structure
covering both address spaces and cv-qual fixes this.

In addition, I want to move cv-qual and address-space-qual onto the
same chain.  That requires fixing a lot of code.

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