Infinite loop in make_cv_type

[Richard Earnshaw <rearnsha@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 2622 bytes --]

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.

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.

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

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

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.

R.

<date>  Richard Earnshaw  (rearnsha@arm.com)

	* gdbtypes.c (make_cv_type): Handle being asked to modify an 
	existing type in the chain.
	(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.




[-- Attachment #2: gdb-gdbtypes.patch --]
[-- Type: text/x-patch , Size: 5226 bytes --]

Index: gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.41
diff -p -r1.41 gdbtypes.c
*** gdbtypes.c	2002/02/08 17:34:33	1.41
--- gdbtypes.c	2002/02/22 11:30:14
*************** alloc_type (struct objfile *objfile)
*** 155,160 ****
--- 155,177 ----
    return (type);
  }
  
+ /* Reset the memory of a type structure.  */
+ struct type *
+ realloc_type (struct type *type, struct objfile *objfile)
+ {
+   memset ((char *) type, 0, sizeof (struct type));
+ 
+   /* Initialize the fields that might not be zero. */
+ 
+   TYPE_CODE (type) = TYPE_CODE_UNDEF;
+   TYPE_OBJFILE (type) = objfile;
+   TYPE_VPTR_FIELDNO (type) = -1;
+   TYPE_CV_TYPE (type) = type;	/* chain back to itself */
+   TYPE_AS_TYPE (type) = type;	/* ditto */
+ 
+   return (type);
+ }
+ 
  /* Lookup a pointer to a type TYPE.  TYPEPTR, if nonzero, points
     to a pointer to memory where the pointer type should be stored.
     If *TYPEPTR is zero, update it to point to the pointer type we return.
*************** make_pointer_type (struct type *type, st
*** 190,197 ****
      {
        ntype = *typeptr;
        objfile = TYPE_OBJFILE (ntype);
!       memset ((char *) ntype, 0, sizeof (struct type));
!       TYPE_OBJFILE (ntype) = objfile;
      }
  
    TYPE_TARGET_TYPE (ntype) = type;
--- 207,213 ----
      {
        ntype = *typeptr;
        objfile = TYPE_OBJFILE (ntype);
!       realloc_type (ntype, objfile);
      }
  
    TYPE_TARGET_TYPE (ntype) = type;
*************** make_reference_type (struct type *type, 
*** 257,264 ****
      {
        ntype = *typeptr;
        objfile = TYPE_OBJFILE (ntype);
!       memset ((char *) ntype, 0, sizeof (struct type));
!       TYPE_OBJFILE (ntype) = objfile;
      }
  
    TYPE_TARGET_TYPE (ntype) = type;
--- 273,279 ----
      {
        ntype = *typeptr;
        objfile = TYPE_OBJFILE (ntype);
!       realloc_type (ntype, objfile);
      }
  
    TYPE_TARGET_TYPE (ntype) = type;
*************** make_function_type (struct type *type, s
*** 306,313 ****
      {
        ntype = *typeptr;
        objfile = TYPE_OBJFILE (ntype);
!       memset ((char *) ntype, 0, sizeof (struct type));
!       TYPE_OBJFILE (ntype) = objfile;
      }
  
    TYPE_TARGET_TYPE (ntype) = type;
--- 321,327 ----
      {
        ntype = *typeptr;
        objfile = TYPE_OBJFILE (ntype);
!       realloc_type (ntype, objfile);
      }
  
    TYPE_TARGET_TYPE (ntype) = type;
*************** make_cv_type (int cnst, int voltl, struc
*** 413,421 ****
    register struct type *tmp_type = type;	/* tmp type */
    struct objfile *objfile;
  
!   ntype = TYPE_CV_TYPE (type);
  
!   while (ntype != type)
      {
        if ((TYPE_CONST (ntype) == cnst) &&
  	  (TYPE_VOLATILE (ntype) == voltl))
--- 427,435 ----
    register struct type *tmp_type = type;	/* tmp type */
    struct objfile *objfile;
  
!   ntype = type;
  
!   do
      {
        if ((TYPE_CONST (ntype) == cnst) &&
  	  (TYPE_VOLATILE (ntype) == voltl))
*************** make_cv_type (int cnst, int voltl, struc
*** 428,436 ****
--- 442,462 ----
  	      return ntype;
  	    }
  	}
+ 
+       /* We have storage and it matches something already in this type.  We
+ 	 must update it.  */
+       if (typeptr && *typeptr == ntype)
+ 	{
+ 	  /* Remove this from the chain (so that we can put it back again
+ 	     below).  */
+ 	  TYPE_CV_TYPE (tmp_type) = TYPE_CV_TYPE (ntype);
+ 	  break;
+ 	}
+ 
        tmp_type = ntype;
        ntype = TYPE_CV_TYPE (ntype);
      }
+   while (ntype != type);
  
    if (typeptr == 0 || *typeptr == 0)	/* We'll need to allocate one.  */
      {
*************** make_cv_type (int cnst, int voltl, struc
*** 467,473 ****
      TYPE_FLAGS (ntype) &= ~TYPE_FLAG_VOLATILE;
  
    /* Fix the chain of cv variants */
!   TYPE_CV_TYPE (ntype) = type;
    TYPE_CV_TYPE (tmp_type) = ntype;
  
    return ntype;
--- 493,499 ----
      TYPE_FLAGS (ntype) &= ~TYPE_FLAG_VOLATILE;
  
    /* Fix the chain of cv variants */
!   TYPE_CV_TYPE (ntype) = TYPE_CV_TYPE (tmp_type);
    TYPE_CV_TYPE (tmp_type) = ntype;
  
    return ntype;
*************** smash_to_member_type (struct type *type,
*** 877,884 ****
  
    objfile = TYPE_OBJFILE (type);
  
!   memset ((char *) type, 0, sizeof (struct type));
!   TYPE_OBJFILE (type) = objfile;
    TYPE_TARGET_TYPE (type) = to_type;
    TYPE_DOMAIN_TYPE (type) = domain;
    TYPE_LENGTH (type) = 1;	/* In practice, this is never needed.  */
--- 903,909 ----
  
    objfile = TYPE_OBJFILE (type);
  
!   realloc_type (type, objfile);
    TYPE_TARGET_TYPE (type) = to_type;
    TYPE_DOMAIN_TYPE (type) = domain;
    TYPE_LENGTH (type) = 1;	/* In practice, this is never needed.  */
*************** smash_to_method_type (struct type *type,
*** 900,907 ****
  
    objfile = TYPE_OBJFILE (type);
  
!   memset ((char *) type, 0, sizeof (struct type));
!   TYPE_OBJFILE (type) = objfile;
    TYPE_TARGET_TYPE (type) = to_type;
    TYPE_DOMAIN_TYPE (type) = domain;
    TYPE_ARG_TYPES (type) = args;
--- 925,931 ----
  
    objfile = TYPE_OBJFILE (type);
  
!   realloc_type (type, objfile);
    TYPE_TARGET_TYPE (type) = to_type;
    TYPE_DOMAIN_TYPE (type) = domain;
    TYPE_ARG_TYPES (type) = args;