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.
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" } } */
