diff mbox

PR63300 'const volatile' sometimes stripped in debug info.

Message ID 20140923114440.GA19566@blokker.redhat.com
State New
Headers show

Commit Message

Mark Wielaard Sept. 23, 2014, 11:44 a.m. UTC 
On Mon, Sep 22, 2014 at 10:59:38AM +0200, Andreas Arnez wrote:
> > +  sub_qual = TYPE_QUAL_RESTRICT;
> > +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
> > +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
> > +    return true;
> > +
> > +  sub_qual = TYPE_QUAL_CONST & TYPE_QUAL_VOLATILE;
> 
> You probably mean '|' instead of '&' here.

Eep. Yes (3x).

But then I tried to write a testcase to show the cases that fail
because of this typo. And failed, everything works fine. That is
because these checks are bogus. sub_quals contains at most 2 qualifiers.
const, volatile and restrict, minus the qual we are interested in.
And subtracting two always results in the empty set. So I removed
those 3 cases.

I added a comment on the top to explain better what we are testing
for. And I included the testcase I wrote. Even though they succeed
before and after the patch. It seems a good sanity check to have in
case we add a new qualifier (atomic).

> IIUC, 'sub_qual' above represents the qualifiers to *omit* from the ones
> we're interested in, right?  Maybe it would be more straightforward to
> reverse the logic, i.e., start with
> 
>         sub_qual = TYPE_QUAL_VOLATILE | TYPE_QUAL_RESTRICT;
> 
> and then always use sub_qual instead of ~sub_qual.

We are interested in the sub_quals minus the given sub_qual indeed
(but not in the empty set). Since we have to mask them off anyway
I found using & ~ more natural.

> Also note that the logic wouldn't scale too well for yet more
> qualifiers...

Yeah. Luckily there is only one more to add (atomic).

Update patch attached.

Thanks,

Mark
PR63300 'const volatile' sometimes stripped in debug info.
    
    When adding DW_TAG_restrict_type I made a mistake when updating the
    code that handled types with multiple modifiers. This patch fixes it
    by putting the logic for finding the "sub-qualified" type in a separate
    function and fall back to adding the modifiers separately if there is
    no such existing type. 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
    
    	* dwarf2out.c (existing_sub_qualified_type): New function.
    	(modified_type_die): Use existing_sub_qualified_type. Fall
    	back to adding modifiers one by one if there is no existing
    	sub-qualified type.
    
    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.

Comments

Andreas Arnez Sept. 23, 2014, 3:17 p.m. UTC | #1 
On Tue, Sep 23 2014, Mark Wielaard wrote:

> On Mon, Sep 22, 2014 at 10:59:38AM +0200, Andreas Arnez wrote:
>> > +  sub_qual = TYPE_QUAL_RESTRICT;
>> > +  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
>> > +      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
>> > +    return true;
>> > +
>> > +  sub_qual = TYPE_QUAL_CONST & TYPE_QUAL_VOLATILE;
>> 
>> You probably mean '|' instead of '&' here.
>
> Eep. Yes (3x).
>
> But then I tried to write a testcase to show the cases that fail
> because of this typo. And failed, everything works fine. That is
> because these checks are bogus. sub_quals contains at most 2 qualifiers.
> const, volatile and restrict, minus the qual we are interested in.
> And subtracting two always results in the empty set. So I removed
> those 3 cases.

Right, the checks were bogus.  But from looking at the code before
adding restrict, I guess it was intended to *minimze* the number of
generated DIEs.  If we keep that goal, maybe the function should
actually return the "rank" of the found sub-qualified type (i.e., the
number of matching sub-qualifiers) and the caller should follow the path
with the highest rank.  Then the checks would be needed again, and they
would even have to be doubled for 'atomic'.  Without such handling there
are cases where more DIEs than necessary are created, e.g. if we have
the following types:

some_base_t *const
some_base_t *volatile restrict
some_base_t *const volatile restrict

Then the latter is based on the first instead of the second, although
this requires one more DIE.
diff mbox

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index e87ade2..b3d2313 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -10461,6 +10461,50 @@  decl_quals (const_tree decl)
 	     ? TYPE_QUAL_VOLATILE : TYPE_UNQUALIFIED));
 }
 
+/* Returns true if CV_QUALS contains QUAL and we have a qualified
+   variant of TYPE that has at least one other qualifier found in
+   CV_QUALS.  Returns false if CV_QUALS doesn't contain QUAL, if
+   CV_QUALS is empty after subtracting QUAL, or if we don't have a
+   TYPE that has at least one qualifier from CV_QUALS minus QUAL.  */
+static bool
+existing_sub_qualified_type (tree type, int cv_quals, int qual)
+{
+  int sub_qual, sub_quals = cv_quals & ~qual;
+  if ((cv_quals & qual) == TYPE_UNQUALIFIED || sub_quals == TYPE_UNQUALIFIED)
+    return false;
+
+  /* There are only three qualifiers handled at the moment that can be
+     combined: const, volatile and restrict.  We are looking for a
+     type that doesn't have one of them (qual) and that has one or
+     more of the others given (sub_quals = cv_quals & ~qual).  We
+     aren't looking for a type that has all qualifiers (cv_quals).  If
+     such a type exists it would have been found already in the caller
+     (modified_type_die).  This means sub_quals will contain at most
+     two qualifiers.  We don't test for the empty set (no quals) since
+     that is not interesting to the caller (then it can pick any qual
+     to add anyway).  */
+
+  if (get_qualified_type (type, sub_quals) != NULL_TREE)
+    return true;
+
+  sub_qual = TYPE_QUAL_CONST;
+  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
+      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
+    return true;
+
+  sub_qual = TYPE_QUAL_VOLATILE;
+  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
+      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
+    return true;
+
+  sub_qual = TYPE_QUAL_RESTRICT;
+  if ((sub_quals & ~sub_qual) != TYPE_UNQUALIFIED
+      && get_qualified_type (type, sub_quals & ~sub_qual) != NULL_TREE)
+    return true;
+
+  return false;
+}
+
 /* Given a pointer to an arbitrary ..._TYPE tree node, return a debugging
    entry that chains various modifiers in front of the given type.  */
 
@@ -10543,34 +10587,48 @@  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)))
+  /* If there are multiple type modifiers, prefer a path which
+     leads to a qualified type.  */
+  if (existing_sub_qualified_type (type, cv_quals, TYPE_QUAL_CONST))
     {
       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)))
+  else if (existing_sub_qualified_type (type, cv_quals, TYPE_QUAL_VOLATILE))
     {
       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)
+  else if (existing_sub_qualified_type (type, 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);
     }
+  else if (cv_quals)
+    {
+      /* No existing path, just add qualifiers one by one.  */
+      if (cv_quals & TYPE_QUAL_CONST)
+	{
+	  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)
+	{
+	  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
+	{
+	  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);
+	}
+    }
   else if (code == POINTER_TYPE)
     {
       mod_type_die = new_die (DW_TAG_pointer_type, mod_scope, 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..7e6220d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-1.c
@@ -0,0 +1,19 @@ 
+/* 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..7484a03
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/stacked-qualified-types-2.c
@@ -0,0 +1,20 @@ 
+/* 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" } } */