Re: [patch] fix exp/12117

[Doug Evans <dje@google.com>]

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

On Wed, Oct 13, 2010 at 2:10 PM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> Also I do not like much the pr[0-9]+.exp names, one gets fimiliar with any
> name but I never remember PR numbers.  [in fact an idea by Roland McGrath]

I often just go with the flow for things such as this, in the absence
of specific rules.
If we want to declare a specific rule of no more such prNNN naming,
that's fine by me.

> @@ -1388,7 +1389,7 @@ check_typedef (struct type *type)
>          /* It is dangerous to call lookup_symbol if we are currently
>             reading a symtab.  Infinite recursion is one danger.  */
>          if (currently_reading_symtab)
> -           return type;
> +           return make_cv_type (is_const, is_volatile, type, NULL);
>
>          name = type_name_no_tag (type);
>          /* FIXME: shouldn't we separately check the TYPE_NAME and

I left this out for a later pass to be conservative and avoid having
to deal with any unintended consequences.
But if one wants to handle this in the same patch, ok.

> @@ -1398,7 +1399,7 @@ check_typedef (struct type *type)
>          if (name == NULL)
>            {
>              stub_noname_complaint ();
> -             return type;
> +             return make_cv_type (is_const, is_volatile, type, NULL);
>            }
>          sym = lookup_symbol (name, 0, STRUCT_DOMAIN, 0);
>          if (sym)

Ditto.

> @@ -1407,11 +1408,10 @@ check_typedef (struct type *type)
>            TYPE_TARGET_TYPE (type) = alloc_type_arch (get_type_arch (type));
>        }
>       type = TYPE_TARGET_TYPE (type);
> +      is_const |= TYPE_CONST (type);
> +      is_volatile |= TYPE_VOLATILE (type);
>     }
>
> -  is_const = TYPE_CONST (type);
> -  is_volatile = TYPE_VOLATILE (type);
> -
>   /* If this is a struct/class/union with no fields, then check
>      whether a full definition exists somewhere else.  This is for
>      systems where a type definition with no fields is issued for such

This part doesn't feel right.
For example, when a stub is later filled in the accumulated cv
qualifiers will get filled in too, but that's wrong.
[Right?]

Plus there's more than just const/volatile to preserve.
[Right?]

I like this patch better.


2010-10-13  Doug Evans  <dje@google.com>
            Jan Kratochvil  <jan.kratochvil@redhat.com>

        PR exp/12117
        * gdbtypes.c (check_typedef): Clean up function comment.
        Keep track of instance flags as we strip typedefs and create a new
        type to preserve them if necessary.

        testsuite/
        * gdb.cp/ptype-cv-cp.cc: New file.
        * gdb.cp/ptype-cv-cp.exp: New file.

[-- Attachment #2: gdb-101013-12117-3.patch.txt --]
[-- Type: text/plain, Size: 9425 bytes --]

2010-10-13  Doug Evans  <dje@google.com>
	    Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR exp/12117
	* gdbtypes.c (check_typedef): Clean up function comment.
	Keep track of instance flags as we strip typedefs and create a new
	type to preserve them if necessary.

	testsuite/
	* gdb.cp/ptype-cv-cp.cc: New file.
	* gdb.cp/ptype-cv-cp.exp: New file.

Index: gdbtypes.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbtypes.c,v
retrieving revision 1.201
diff -u -p -r1.201 gdbtypes.c
--- gdbtypes.c	12 Oct 2010 20:58:17 -0000	1.201
+++ gdbtypes.c	14 Oct 2010 01:44:11 -0000
@@ -1347,7 +1347,21 @@ stub_noname_complaint (void)
   complaint (&symfile_complaints, _("stub type has NULL name"));
 }
 
-/* Added by Bryan Boreham, Kewill, Sun Sep 17 18:07:17 1989.
+/* Find the real type of TYPE.  This function returns the real type,
+   after removing all layers of typedefs, and completing opaque or stub
+   types.  Completion changes the TYPE argument, but stripping of
+   typedefs does not.
+
+   Instance flags (e.g. const/volatile) are preserved as typedefs are
+   stripped.  If necessary a new qualified form of the underlying type
+   is created.
+
+   NOTE: This will return a typedef if TYPE_TARGET_TYPE for the typedef has
+   not been computed and we're either in the middle of reading symbols, or
+   there was no name for the typedef in the debug info.
+
+   If TYPE is a TYPE_CODE_TYPEDEF, its length is updated to the length of
+   the target type.
 
    If this is a stubbed struct (i.e. declared as struct foo *), see if
    we can find a full definition in some other file. If so, copy this
@@ -1355,26 +1369,15 @@ stub_noname_complaint (void)
    (but not any code) that if we don't find a full definition, we'd
    set a flag so we don't spend time in the future checking the same
    type.  That would be a mistake, though--we might load in more
-   symbols which contain a full definition for the type.
-
-   This used to be coded as a macro, but I don't think it is called 
-   often enough to merit such treatment.
-
-   Find the real type of TYPE.  This function returns the real type,
-   after removing all layers of typedefs and completing opaque or stub
-   types.  Completion changes the TYPE argument, but stripping of
-   typedefs does not.
-
-   If TYPE is a TYPE_CODE_TYPEDEF, its length is (also) set to the length of
-   the target type instead of zero.  However, in the case of TYPE_CODE_TYPEDEF
-   check_typedef can still return different type than the original TYPE
-   pointer.  */
+   symbols which contain a full definition for the type.  */
 
 struct type *
 check_typedef (struct type *type)
 {
   struct type *orig_type = type;
-  int is_const, is_volatile;
+  /* While we're removing typedefs, we don't want to lose qualifiers.
+     E.g., const/volatile.  */
+  int instance_flags = TYPE_INSTANCE_FLAGS (type);
 
   gdb_assert (type);
 
@@ -1388,7 +1391,7 @@ check_typedef (struct type *type)
 	  /* It is dangerous to call lookup_symbol if we are currently
 	     reading a symtab.  Infinite recursion is one danger.  */
 	  if (currently_reading_symtab)
-	    return type;
+	    return make_qualified_type (type, instance_flags, NULL);
 
 	  name = type_name_no_tag (type);
 	  /* FIXME: shouldn't we separately check the TYPE_NAME and
@@ -1398,7 +1401,7 @@ check_typedef (struct type *type)
 	  if (name == NULL)
 	    {
 	      stub_noname_complaint ();
-	      return type;
+	      return make_qualified_type (type, instance_flags, NULL);
 	    }
 	  sym = lookup_symbol (name, 0, STRUCT_DOMAIN, 0);
 	  if (sym)
@@ -1407,10 +1410,33 @@ check_typedef (struct type *type)
 	    TYPE_TARGET_TYPE (type) = alloc_type_arch (get_type_arch (type));
 	}
       type = TYPE_TARGET_TYPE (type);
-    }
 
-  is_const = TYPE_CONST (type);
-  is_volatile = TYPE_VOLATILE (type);
+      /* Preserve the instance flags as we traverse down the typedef chain.
+
+	 Handling address spaces/classes is nasty, what do we do if there's a
+	 conflict?
+	 E.g., what if an outer typedef marks the type as class_1 and an inner
+	 typedef marks the type as class_2?
+	 This is the wrong place to do such error checking.  We leave it to
+	 the code that created the typedef in the first place to flag the
+	 error.  We just pick the outer address space (akin to letting the
+	 outer cast in a chain of casting win), instead of assuming
+	 "it can't happen".  */
+      {
+	const int ALL_SPACES = (TYPE_INSTANCE_FLAG_CODE_SPACE
+				| TYPE_INSTANCE_FLAG_DATA_SPACE);
+	const int ALL_CLASSES = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
+	int new_instance_flags = TYPE_INSTANCE_FLAGS (type);
+
+	/* Treat code vs data spaces and address classes separately.  */
+	if ((instance_flags & ALL_SPACES) != 0)
+	  new_instance_flags &= ~ALL_SPACES;
+	if ((instance_flags & ALL_CLASSES) != 0)
+	  new_instance_flags &= ~ALL_CLASSES;
+
+	instance_flags |= new_instance_flags;
+      }
+    }
 
   /* If this is a struct/class/union with no fields, then check
      whether a full definition exists somewhere else.  This is for
@@ -1428,7 +1454,7 @@ check_typedef (struct type *type)
       if (name == NULL)
 	{
 	  stub_noname_complaint ();
-	  return type;
+	  return make_qualified_type (type, instance_flags, NULL);
 	}
       newtype = lookup_transparent_type (name);
 
@@ -1445,7 +1471,9 @@ check_typedef (struct type *type)
 	     move over any other types NEWTYPE refers to, which could
 	     be an unbounded amount of stuff.  */
 	  if (TYPE_OBJFILE (newtype) == TYPE_OBJFILE (type))
-	    make_cv_type (is_const, is_volatile, newtype, &type);
+	    type = make_qualified_type (newtype,
+					TYPE_INSTANCE_FLAGS (type),
+					type);
 	  else
 	    type = newtype;
 	}
@@ -1464,17 +1492,18 @@ check_typedef (struct type *type)
       if (name == NULL)
 	{
 	  stub_noname_complaint ();
-	  return type;
+	  return make_qualified_type (type, instance_flags, NULL);
 	}
       sym = lookup_symbol (name, 0, STRUCT_DOMAIN, 0);
       if (sym)
         {
           /* Same as above for opaque types, we can replace the stub
-             with the complete type only if they are int the same
+             with the complete type only if they are in the same
              objfile.  */
 	  if (TYPE_OBJFILE (SYMBOL_TYPE(sym)) == TYPE_OBJFILE (type))
-            make_cv_type (is_const, is_volatile, 
-			  SYMBOL_TYPE (sym), &type);
+            type = make_qualified_type (SYMBOL_TYPE (sym),
+					TYPE_INSTANCE_FLAGS (type),
+					type);
 	  else
 	    type = SYMBOL_TYPE (sym);
         }
@@ -1534,8 +1563,12 @@ check_typedef (struct type *type)
 	  TYPE_TARGET_STUB (type) = 0;
 	}
     }
+
+  type = make_qualified_type (type, instance_flags, NULL);
+
   /* Cache TYPE_LENGTH for future use.  */
   TYPE_LENGTH (orig_type) = TYPE_LENGTH (type);
+
   return type;
 }
 
Index: testsuite/gdb.cp/ptype-cv-cp.cc
===================================================================
RCS file: testsuite/gdb.cp/ptype-cv-cp.cc
diff -N testsuite/gdb.cp/ptype-cv-cp.cc
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/ptype-cv-cp.cc	14 Oct 2010 00:33:32 -0000
@@ -0,0 +1,17 @@
+typedef int my_int;
+typedef const my_int const_my_int;
+typedef volatile my_int volatile_my_int;
+typedef volatile const_my_int volatile_const_my_int;
+typedef const volatile_my_int const_volatile_my_int;
+
+my_int v_my_int (0);
+const_my_int v_const_my_int (1);
+volatile_my_int v_volatile_my_int (2);
+const_volatile_my_int v_const_volatile_my_int (3);
+volatile_const_my_int v_volatile_const_my_int (4);
+
+int
+main ()
+{
+  return 0;
+}
Index: testsuite/gdb.cp/ptype-cv-cp.exp
===================================================================
RCS file: testsuite/gdb.cp/ptype-cv-cp.exp
diff -N testsuite/gdb.cp/ptype-cv-cp.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.cp/ptype-cv-cp.exp	14 Oct 2010 00:33:32 -0000
@@ -0,0 +1,41 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+if { [skip_cplus_tests] } { continue }
+
+set testfile "ptype-cv-cp"
+set srcfile ${testfile}.cc
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug c++}] } {
+    return -1
+}
+
+gdb_test "whatis v_my_int" "my_int"
+gdb_test "ptype v_my_int" "int"
+
+gdb_test "whatis v_const_my_int" "const_my_int"
+gdb_test "ptype v_const_my_int" "const int"
+
+gdb_test "whatis v_volatile_my_int" "volatile_my_int"
+gdb_test "ptype v_volatile_my_int" "volatile int"
+
+gdb_test "whatis v_const_volatile_my_int" "const_volatile_my_int"
+gdb_test "ptype v_const_volatile_my_int" "const volatile int"
+
+gdb_test "whatis v_volatile_const_my_int" "volatile_const_my_int"
+setup_kfail "gcc/45997" *-*-*
+gdb_test "ptype v_volatile_const_my_int" "const volatile int"