From patchwork Tue Sep 23 11:44:40 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Wielaard X-Patchwork-Id: 392435 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id BDD131400B5 for ; Tue, 23 Sep 2014 21:44:54 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=pZbcfQS7LDaJoy5kS aOf8ckIrSh0nldQs4wQQnQGIZxcSzzVB5oypdpKuI7lqFzAF66g1f25/gFW/o9HN yQkJuE3fUyuOEnVkaAOcMiy+dDNOs3a778/ZbdHDBLf2nqIMiu2u057QceuJeGhB zGmoxTYiNSphP+3dj+OLArIW88= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=DgFtwnH7WsFoBFQ3DXodJdH btOs=; b=Soo4rpp/cv8o+yfgrG6zhlHhqQHo2C6KMWwcfNmv//OU8EPAAU3NWQ+ 4p7n11NFaZmunHZ0eXZ6ePpKlIGOzDvejn14bo6aRP8tFf1ud6H8aSIZcXlzgqS5 Mvm2Jve5n5FPiL/Rgse0V5LO+7fAAu/ymgUvityaFRhZp3kIJs9s= Received: (qmail 17791 invoked by alias); 23 Sep 2014 11:44:47 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 17741 invoked by uid 89); 23 Sep 2014 11:44:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.0 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 23 Sep 2014 11:44:45 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s8NBih1L021123 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 23 Sep 2014 07:44:43 -0400 Received: from blokker.wildebeest.org (ovpn-116-103.ams2.redhat.com [10.36.116.103]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s8NBif0f009891 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 23 Sep 2014 07:44:42 -0400 Received: by blokker.wildebeest.org (Postfix, from userid 1000) id BE73320277; Tue, 23 Sep 2014 13:44:40 +0200 (CEST) Date: Tue, 23 Sep 2014 13:44:40 +0200 From: Mark Wielaard To: Andreas Arnez Cc: gcc-patches@gcc.gnu.org, jason@redhat.com Subject: Re: [PATCH] PR63300 'const volatile' sometimes stripped in debug info. Message-ID: <20140923114440.GA19566@blokker.redhat.com> References: <1411249535-2193-1-git-send-email-mjw@redhat.com> <87oau8kp39.fsf@br87z6lw.de.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87oau8kp39.fsf@br87z6lw.de.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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" } } */