Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info.

[Andreas Arnez <arnez at linux dot vnet dot ibm dot com>]

On Wed, Sep 24 2014, Jakub Jelinek wrote:

> On Wed, Sep 24, 2014 at 02:40:14PM +0200, Andreas Arnez wrote:
>> A few style aspects I'm not sure about:
>> 
>> * Is it OK to use __builtin_popcount in tree.c?
>
> Definitely not, you can use popcount_hwi instead, which for GCC
> host compiler (>= 3.4) will use __builtin_popcount*, otherwise
> fallback to a library function.
>
>> * Is it acceptable to add such a specialized function as
>>   get_nearest_type_subqualifiers to the tree interface?  Or would it be
>>   preferable to move it as a static function to dwarf2out.c, since
>>   that's the only user right now?
>
> I agree it should be kept in dwarf2out.c, it is too specialized.
>
> 	Jakub

OK, I'm using popcount_hwi now and moved get_nearest_type_subqualifiers
to dwarf2out.c.  Does this look OK?

-- >8 --
Subject: [PATCH v4] PR63300 'const volatile' sometimes stripped in debug info.

When adding DW_TAG_restrict_type the handling of multiple modifiers
was adjusted incorrectly.  This patch fixes it with the help of a new
tree function get_nearest_type_subqualifiers.  The old tests didn't
catch this case because there always was an existing sub-qualified
type already.  The new guality testcase fails before and succeeds
after this patch.  The new dwarf2 testcases make sure the optimization
works and doesn't introduce unnecessary type tags.

gcc/ChangeLog

	* tree.c (check_base_type): New.
	(check_qualified_type): Exploit new helper function above.
	* tree.h (check_base_type): New prototype.
	* dwarf2out.c (get_nearest_type_subqualifiers): New.
	(modified_type_die): Fix handling for qualifiers.  Qualifiers to
	"peel off" are now determined using get_nearest_type_subqualifiers.

gcc/testsuite/ChangeLog

	* gcc.dg/debug/dwarf2/stacked-qualified-types-1.c: New testcase.
	* gcc.dg/debug/dwarf2/stacked-qualified-types-2.c: Likewise.
	* gcc.dg/guality/pr63300-const-volatile.c: New testcase.
---
 gcc/dwarf2out.c                                    | 96 +++++++++++++++-------
 .../debug/dwarf2/stacked-qualified-types-1.c       | 18 ++++
 .../debug/dwarf2/stacked-qualified-types-2.c       | 19 +++++
 .../gcc.dg/guality/pr63300-const-volatile.c        | 12 +++
 gcc/tree.c                                         | 16 +++-
 gcc/tree.h                                         |  4 +
 6 files changed, 131 insertions(+), 34 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-1.c
 create mode 100644 gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-2.c
 create mode 100644 gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index e87ade2..e15b42b 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -10461,6 +10461,40 @@ decl_quals (const_tree decl)
 	     ? TYPE_QUAL_VOLATILE : TYPE_UNQUALIFIED));
 }
 
+/* Determine the TYPE whose qualifiers match the largest strict subset
+   of the given TYPE_QUALS, and return its qualifiers.  Ignore all
+   qualifiers outside QUAL_MASK.  */
+
+static int
+get_nearest_type_subqualifiers (tree type, int type_quals, int qual_mask)
+{
+  tree t;
+  int best_rank = 0, best_qual = 0, max_rank;
+
+  type_quals &= qual_mask;
+  max_rank = popcount_hwi (type_quals) - 1;
+
+  for (t = TYPE_MAIN_VARIANT (type); t && best_rank < max_rank;
+       t = TYPE_NEXT_VARIANT (t))
+    {
+      int q = TYPE_QUALS (t) & qual_mask;
+
+      if ((q & type_quals) == q && q != type_quals
+	  && check_base_type (t, type))
+	{
+	  int rank = popcount_hwi (q);
+
+	  if (rank > best_rank)
+	    {
+	      best_rank = rank;
+	      best_qual = q;
+	    }
+	}
+    }
+
+  return best_qual;
+}
+
 /* Given a pointer to an arbitrary ..._TYPE tree node, return a debugging
    entry that chains various modifiers in front of the given type.  */
 
@@ -10474,12 +10508,14 @@ modified_type_die (tree type, int cv_quals, dw_die_ref context_die)
   tree qualified_type;
   tree name, low, high;
   dw_die_ref mod_scope;
+  /* Only these cv-qualifiers are currently handled.  */
+  const int cv_qual_mask = (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE
+			    | TYPE_QUAL_RESTRICT);
 
   if (code == ERROR_MARK)
     return NULL;
 
-  /* Only these cv-qualifiers are currently handled.  */
-  cv_quals &= (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT);
+  cv_quals &= cv_qual_mask;
 
   /* Don't emit DW_TAG_restrict_type for DWARFv2, since it is a type
      tag modifier (and not an attribute) old consumers won't be able
@@ -10530,7 +10566,7 @@ modified_type_die (tree type, int cv_quals, dw_die_ref context_die)
       else
 	{
 	  int dquals = TYPE_QUALS_NO_ADDR_SPACE (dtype);
-	  dquals &= (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT);
+	  dquals &= cv_qual_mask;
 	  if ((dquals & ~cv_quals) != TYPE_UNQUALIFIED
 	      || (cv_quals == dquals && DECL_ORIGINAL_TYPE (name) != type))
 	    /* cv-unqualified version of named type.  Just use
@@ -10543,33 +10579,33 @@ modified_type_die (tree type, int cv_quals, dw_die_ref context_die)
 
   mod_scope = scope_die_for (type, context_die);
 
-  if ((cv_quals & TYPE_QUAL_CONST)
-      /* If there are multiple type modifiers, prefer a path which
-	 leads to a qualified type.  */
-      && (((cv_quals & ~TYPE_QUAL_CONST) == TYPE_UNQUALIFIED)
-	  || get_qualified_type (type, cv_quals) == NULL_TREE
-	  || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_CONST)
-	      != NULL_TREE)))
-    {
-      mod_type_die = new_die (DW_TAG_const_type, mod_scope, type);
-      sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_CONST,
-				   context_die);
-    }
-  else if ((cv_quals & TYPE_QUAL_VOLATILE)
-	   && (((cv_quals & ~TYPE_QUAL_VOLATILE) == TYPE_UNQUALIFIED)
-	       || get_qualified_type (type, cv_quals) == NULL_TREE
-	       || (get_qualified_type (type, cv_quals & ~TYPE_QUAL_VOLATILE)
-		   != NULL_TREE)))
-    {
-      mod_type_die = new_die (DW_TAG_volatile_type, mod_scope, type);
-      sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_VOLATILE,
-				   context_die);
-    }
-  else if (cv_quals & TYPE_QUAL_RESTRICT)
-    {
-      mod_type_die = new_die (DW_TAG_restrict_type, mod_scope, type);
-      sub_die = modified_type_die (type, cv_quals & ~TYPE_QUAL_RESTRICT,
-				   context_die);
+  if (cv_quals)
+    {
+      struct qual_info { int q; enum dwarf_tag t; };
+      static const struct qual_info qual_info[] =
+	{
+	  { TYPE_QUAL_RESTRICT, DW_TAG_restrict_type },
+	  { TYPE_QUAL_VOLATILE, DW_TAG_volatile_type },
+	  { TYPE_QUAL_CONST, DW_TAG_const_type },
+	};
+      int sub_quals;
+      unsigned i;
+
+      /* Determine a lesser qualified type that most closely matches
+	 this one.  Then generate DW_TAG_* entries for the remaining
+	 qualifiers.  */
+      sub_quals = get_nearest_type_subqualifiers (type, cv_quals,
+						  cv_qual_mask);
+      mod_type_die = modified_type_die (type, sub_quals, context_die);
+
+      for (i = 0; i < sizeof (qual_info) / sizeof (qual_info[0]); i++)
+	if (qual_info[i].q & cv_quals & ~sub_quals)
+	  {
+	    dw_die_ref d = new_die (qual_info[i].t, mod_scope, type);
+	    if (mod_type_die)
+	      add_AT_die_ref (d, DW_AT_type, mod_type_die);
+	    mod_type_die = d;
+	  }
     }
   else if (code == POINTER_TYPE)
     {
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-1.c b/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-1.c
new file mode 100644
index 0000000..6f40901
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-1.c
@@ -0,0 +1,18 @@
+/* PR63300 make sure we don't duplicate type qualifiers unneeded.  */
+/* { dg-do compile } */
+/* { dg-options "-gdwarf -dA" } */
+
+/* This should give us:
+   - One const type pointing to a char
+   - One volatile type pointing to a char
+   - Either one const type pointing to the volatile type pointing to a char
+     or one volatile type pointing to the const type pointing to a char.
+     But not both.  */
+
+char a;
+const char b;
+volatile const char c;
+volatile char d;
+const volatile char e;
+
+/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\) DW_TAG_(?:const|volatile)_type" 3 } } */
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-2.c b/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-2.c
new file mode 100644
index 0000000..5a8d3a0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-2.c
@@ -0,0 +1,19 @@
+/* PR63300 make sure we don't duplicate type qualifiers unneeded.  */
+/* { dg-do compile } */
+/* { dg-options "-std=c99 -gdwarf-4 -dA" } */
+
+/* This should give us:
+   - One restrict type pointing to a char pointer.
+   - One volatile type pointing to the restrict type.
+   - One const type pointing to the restrict type.
+   - Either one const type pointing to the volatile type pointing to
+     the restrict type or one volatile type pointing to the const type
+     pointing to the restrict type.  But not both.  */
+
+char * restrict a;
+char * const restrict b;
+char * const volatile restrict c;
+char * volatile restrict d;
+
+/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\) DW_TAG_restrict_type" 1 } } */
+/* { dg-final { scan-assembler-times "DIE \\(\[^\n\]*\\) DW_TAG_(?:const|volatile)_type" 3 } } */
diff --git a/gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c b/gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
new file mode 100644
index 0000000..b8d75ed
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr63300-const-volatile.c
@@ -0,0 +1,12 @@
+/* PR63300 'const volatile' sometimes stripped in debug info */
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+int
+main (int argc, char **argv)
+{
+  const volatile int v = argc;
+  return v - argc;
+}
+
+/* { dg-final { gdb-test 9 "type:v" "const volatile int" } } */
diff --git a/gcc/tree.c b/gcc/tree.c
index 83df030..d999099 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -6195,13 +6195,12 @@ set_type_quals (tree type, int type_quals)
   TYPE_ADDR_SPACE (type) = DECODE_QUAL_ADDR_SPACE (type_quals);
 }
 
-/* Returns true iff CAND is equivalent to BASE with TYPE_QUALS.  */
+/* Returns true iff unqualified CAND and BASE are equivalent.  */
 
 bool
-check_qualified_type (const_tree cand, const_tree base, int type_quals)
+check_base_type (const_tree cand, const_tree base)
 {
-  return (TYPE_QUALS (cand) == type_quals
-	  && TYPE_NAME (cand) == TYPE_NAME (base)
+  return (TYPE_NAME (cand) == TYPE_NAME (base)
 	  /* Apparently this is needed for Objective-C.  */
 	  && TYPE_CONTEXT (cand) == TYPE_CONTEXT (base)
 	  /* Check alignment.  */
@@ -6210,6 +6209,15 @@ check_qualified_type (const_tree cand, const_tree base, int type_quals)
 				   TYPE_ATTRIBUTES (base)));
 }
 
+/* Returns true iff CAND is equivalent to BASE with TYPE_QUALS.  */
+
+bool
+check_qualified_type (const_tree cand, const_tree base, int type_quals)
+{
+  return (TYPE_QUALS (cand) == type_quals
+	  && check_base_type (cand, base));
+}
+
 /* Returns true iff CAND is equivalent to BASE with ALIGN.  */
 
 static bool
diff --git a/gcc/tree.h b/gcc/tree.h
index 93a12d4..f4f399f 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -3829,6 +3829,10 @@ extern tree merge_dllimport_decl_attributes (tree, tree);
 extern tree handle_dll_attribute (tree *, tree, tree, int, bool *);
 #endif
 
+/* Returns true iff unqualified CAND and BASE are equivalent.  */
+
+extern bool check_base_type (const_tree cand, const_tree base);
+
 /* Check whether CAND is suitable to be returned from get_qualified_type
    (BASE, TYPE_QUALS).  */
 
-- 
1.8.4.2