From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id AE3863857C52 for ; Thu, 13 May 2021 00:27:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AE3863857C52 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-463-B0s2qRZkPEuSP5rNI6KZmA-1; Wed, 12 May 2021 20:27:21 -0400 X-MC-Unique: B0s2qRZkPEuSP5rNI6KZmA-1 Received: by mail-qv1-f70.google.com with SMTP id b1-20020a0c9b010000b02901c4bcfbaa53so20251179qve.19 for ; Wed, 12 May 2021 17:27:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=8IZl7XHEydQP7bZRLnUEy707ICW9Nwx48EaFURdP2tA=; b=luHHWEKOGz0DRNdt/CqDwSo3ZK9uD34SolqVPyVlCI9h/3IGNxz7YOFnVZEaJV0ilP GijyBxwTy2C4mjkg9hEqWPDsrIz4oIFfoi21Ww9TVQeBONBpWz0b38Hx4ButEpa/Clhq 5IsCgd/xUJQZAW4KssbomG+EAL0rrz3bCvTAH2bsRm3SMjhGB8TC0J/oS9XXGhwl2ByL /7plx8WSeiiJBpAxTglU4YpbDQsFGzorrdcw+qrihqBYTajzl7JzSrRL5vugnKLJB5xw 3nsmYGXhO6hGdzlkFnnBM5C+/wwq0svQFSNwxb01mxjMfkNXwhWJuTu7pa4h+ow6TenO qPjw== X-Gm-Message-State: AOAM532MJ79jxh9NdFSmuGMDBAP4ZMgSjC4gFthkh3Yg6t90cNtTIqZu 8ZgorNhHLhGxxIeoS2LJn+d3l8zvZS9KNXd95pmPI1tSbZjbVBKKgWHY749BnySFNVxAPC3L36I BXIEohJW4zgUC12c5GLtxWNR8aInU2w4ztX+bl9FzYJabmtwpN6HtOVq4Zb6OVPSA0A== X-Received: by 2002:a0c:f88d:: with SMTP id u13mr38651146qvn.13.1620865640448; Wed, 12 May 2021 17:27:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxY6BYW29hvB8t+n1mQQmVuhNRIxVD9jIG48VG8nU9BuImpAdVaYvTcyIgnr/KDq21PS8lKHQ== X-Received: by 2002:a0c:f88d:: with SMTP id u13mr38651121qvn.13.1620865640004; Wed, 12 May 2021 17:27:20 -0700 (PDT) Received: from [192.168.1.148] ([130.44.159.43]) by smtp.gmail.com with ESMTPSA id o5sm1277570qtl.85.2021.05.12.17.27.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 May 2021 17:27:19 -0700 (PDT) Subject: Re: [PATCH v2] c++: Check attributes on friend declarations [PR99032] To: Marek Polacek Cc: GCC Patches References: <20210512024503.398582-1-polacek@redhat.com> <54dfab57-27f6-793f-0ae6-5d72c0faf531@redhat.com> From: Jason Merrill Message-ID: <38e5679b-4967-26a7-5110-1f17e5c51ef0@redhat.com> Date: Wed, 12 May 2021 20:27:18 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 13 May 2021 00:27:26 -0000 On 5/12/21 8:03 PM, Marek Polacek wrote: > On Wed, May 12, 2021 at 12:21:26PM -0400, Jason Merrill wrote: >> On 5/12/21 11:03 AM, Marek Polacek wrote: >>> On Wed, May 12, 2021 at 10:37:50AM -0400, Jason Merrill wrote: >>>> On 5/11/21 10:45 PM, Marek Polacek wrote: >>>>> This patch implements [dcl.attr.grammar]/5: "If an attribute-specifier-seq >>>>> appertains to a friend declaration ([class.friend]), that declaration shall >>>>> be a definition." >>>>> >>>>> This restriction only applies to C++11-style attributes. There are >>>>> various forms of friend declarations, we have friend templates, C++11 >>>>> extended friend declarations, and so on. In some cases we already >>>>> ignore the attribute and warn that it was ignored. But certain cases >>>>> weren't diagnosed, and with this patch we'll give a hard error. I tried >>>>> hard not to emit both a warning and error and I think it worked out. >>>> >>>> Hmm, why error in this case rather than the usual warning? >>> >>> I thought such usage (C++11 attribute on a friend) is so rare (no occurrences >>> in the testsuite) that it'd be better to go straight for an error. Though >>> I do see the argument that generally we ignore unknown/misplaced attributes. >>> clang gives errors, FWIW. >> >> Up to you. >> >>>> Also, we should warn about similarly useless GNU attributes on friends. >>> >>> That's unfortunately more complicated: we should(?) accept >>> >>> #define vector __attribute__((vector_size(16))) >>> class A { >>> friend vector float f(); >>> }; >>> >>> vector float vf; >>> vector float >>> f () >>> { >>> return vf; >>> } >> >> Ah, sure. Because of the DWIM binding of GNU attributes, that attribute >> applies to 'float' rather than to the friend. >> >> I think we could work around this issue in the grokdeclarator change by >> allowing attributes that have the type_required flag. > > That works (and I need to scan all the attributes, hence the new function > any_type_req_attributes_p). > >> I don't think this issue affects the other two hunks of the patch, so we >> should be able to remove the cxx11_attribute_p checks in those. > > Yeah, done. > >> In the cp_parser_member_declaration change, I don't think we need to look at >> decl_specifiers.std_attributes, as those appertain to the type. > > Right, it only caught the 'friend [[deprecated]] void;' case that's handled > by your patch now. Dropped. > >> Incidentally, I think one of the lines in your testcase is a syntax error >> that we fail to diagnose: >> >> + friend [[deprecated]] void; // { dg-error "attribute appertains" } >> >> I think a C++11 attribute cannot appear in the middle of the >> decl-specifier-seq, only before it (in which case it appertains to the >> declaration) or at the end (in which case it appertains to the type). So we >> should give an error for this line, but not this error. With something >> like: > > Nice, thanks! I've moved that line into a separate test. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > This patch implements [dcl.attr.grammar]/5: "If an attribute-specifier-seq > appertains to a friend declaration ([class.friend]), that declaration shall > be a definition." > > This restriction applies to C++11-style attributes as well as GNU > attributes with the exception that we allow GNU attributes that require > a type, such as vector_size to continue accepting code as in attrib63.C. > There are various forms of friend declarations, we have friend > templates, C++11 extended friend declarations, and so on. In some cases > we already ignore the attribute and warn that it was ignored. But > certain cases weren't diagnosed, and with this patch we'll give a hard > error. I tried hard not to emit both a warning and error and I think it > worked out. > > Jason provided the cp_parser_decl_specifier_seq hunk to detect using > standard attributes in the middle of decl-specifiers, which is invalid. > > Co-authored-by: Jason Merrill > > gcc/cp/ChangeLog: > > PR c++/99032 > * cp-tree.h (any_type_req_attributes_p): Declare. > * decl.c (grokdeclarator): Diagnose when an attribute appertains to > a friend declaration that is not a definition. > * decl2.c (any_type_req_attributes_p): New. > * parser.c (cp_parser_decl_specifier_seq): Diagnose standard attributes > in the middle of decl-specifiers. > (cp_parser_elaborated_type_specifier): Diagnose when an attribute > appertains to a friend declaration that is not a definition. > (cp_parser_member_declaration): Likewise. > > gcc/testsuite/ChangeLog: > > PR c++/99032 > * g++.dg/cpp0x/friend7.C: New test. > * g++.dg/cpp0x/gen-attrs-4.C: Add dg-error. > * g++.dg/cpp0x/gen-attrs-39-1.C: Likewise. > * g++.dg/cpp0x/gen-attrs-74.C: New test. > * g++.dg/ext/attrib63.C: New test. > --- > gcc/cp/cp-tree.h | 1 + > gcc/cp/decl.c | 5 +++ > gcc/cp/decl2.c | 14 ++++++++ > gcc/cp/parser.c | 23 +++++++++++- > gcc/testsuite/g++.dg/cpp0x/friend7.C | 40 +++++++++++++++++++++ > gcc/testsuite/g++.dg/cpp0x/gen-attrs-39-1.C | 3 +- > gcc/testsuite/g++.dg/cpp0x/gen-attrs-4.C | 3 +- > gcc/testsuite/g++.dg/cpp0x/gen-attrs-74.C | 10 ++++++ > gcc/testsuite/g++.dg/ext/attrib63.C | 28 +++++++++++++++ > 9 files changed, 124 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/friend7.C > create mode 100644 gcc/testsuite/g++.dg/cpp0x/gen-attrs-74.C > create mode 100644 gcc/testsuite/g++.dg/ext/attrib63.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index 122dadf976f..f4be943139d 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -6763,6 +6763,7 @@ extern tree grokbitfield (const cp_declarator *, cp_decl_specifier_seq *, > tree, tree, tree); > extern tree splice_template_attributes (tree *, tree); > extern bool any_dependent_type_attributes_p (tree); > +extern bool any_type_req_attributes_p (tree); > extern tree cp_reconstruct_complex_type (tree, tree); > extern bool attributes_naming_typedef_ok (tree); > extern void cplus_decl_attributes (tree *, tree, int); > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c > index bc3928d7f85..ef18cbe6a93 100644 > --- a/gcc/cp/decl.c > +++ b/gcc/cp/decl.c > @@ -13741,6 +13741,11 @@ grokdeclarator (const cp_declarator *declarator, > > if (friendp) > { > + if (attrlist && *attrlist && !funcdef_flag > + /* Hack to allow attributes like vector_size on a friend. */ > + && !any_type_req_attributes_p (*attrlist)) > + error_at (id_loc, "attribute appertains to a friend " > + "declaration that is not a definition"); > /* Friends are treated specially. */ > if (ctype == current_class_type) > ; /* We already issued a permerror. */ > diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c > index 89f874a32cc..2bcefb619aa 100644 > --- a/gcc/cp/decl2.c > +++ b/gcc/cp/decl2.c > @@ -1331,6 +1331,20 @@ any_dependent_type_attributes_p (tree attrs) > return false; > } > > +/* True if ATTRS contains any attribute that requires a type. */ Let's invert this to check if ATTRS contains any attribute that does *not* require a type, and would therefore apply to the decl. > +bool > +any_type_req_attributes_p (tree attrs) > +{ > + for (tree a = attrs; a; a = TREE_CHAIN (a)) > + { > + const attribute_spec *as = lookup_attribute_spec (get_attribute_name (a)); > + if (as && as->type_required) > + return true; > + } > + return false; > +} > + > /* Return true iff ATTRS are acceptable attributes to be applied in-place > to a typedef which gives a previously unnamed class or enum a name for > linkage purposes. */ > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index 41df5dd525f..c0b57955954 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -15146,6 +15146,16 @@ cp_parser_decl_specifier_seq (cp_parser* parser, > if (!found_decl_spec) > break; > > + if (decl_specs->std_attributes) > + { > + error_at (decl_specs->locations[ds_std_attribute], > + "standard attributes in middle of decl-specifiers"); > + inform (decl_specs->locations[ds_std_attribute], > + "standard attributes must precede the decl-specifiers to " > + "apply to the declaration, or follow them to apply to " > + "the type"); > + } > + > decl_specs->any_specifiers_p = true; > /* After we see one decl-specifier, further decl-specifiers are > always optional. */ > @@ -19764,11 +19774,15 @@ cp_parser_elaborated_type_specifier (cp_parser* parser, > && ! processing_explicit_instantiation) > warning (OPT_Wattributes, > "attributes ignored on template instantiation"); > + else if (is_friend && attributes) > + error ("attribute appertains to a friend declaration that is not " > + "a definition"); > else if (is_declaration && cp_parser_declares_only_class_p (parser)) > cplus_decl_attributes (&type, attributes, (int) ATTR_FLAG_TYPE_IN_PLACE); > else > warning (OPT_Wattributes, > - "attributes ignored on elaborated-type-specifier that is not a forward declaration"); > + "attributes ignored on elaborated-type-specifier that is " > + "not a forward declaration"); > } > > if (tag_type == enum_type) > @@ -26054,6 +26068,13 @@ cp_parser_member_declaration (cp_parser* parser) > error_at (decl_spec_token_start->location, > "friend declaration does not name a class or " > "function"); > + /* Give an error if an attribute cannot appear here, as per > + [dcl.attr.grammar]/5. But not when declares_class_or_enum: > + we ignore attributes in elaborated-type-specifiers. */ > + else if (!declares_class_or_enum && decl_specifiers.attributes) > + error_at (decl_spec_token_start->location, > + "attribute appertains to a friend declaration " > + "that is not a definition"); > else > make_friend_class (current_class_type, type, > /*complain=*/true); > diff --git a/gcc/testsuite/g++.dg/cpp0x/friend7.C b/gcc/testsuite/g++.dg/cpp0x/friend7.C > new file mode 100644 > index 00000000000..0308cb49be0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/friend7.C > @@ -0,0 +1,40 @@ > +// PR c++/99032 > +// { dg-do compile { target c++11 } } > + > +class X { }; > +template > +void foo (T1, T2); > + > +struct S { > + [[deprecated]] friend void f(); // { dg-error "attribute appertains" } > + [[deprecated]] friend void f2() { } > + __attribute__((deprecated)) friend void f3(); // { dg-error "attribute appertains" } > + friend void f3 [[deprecated]] (); // { dg-error "attribute appertains" } > + friend void f4 [[deprecated]] () { } > + [[deprecated]] friend void; // { dg-error "attribute appertains" } > + __attribute__((deprecated)) friend int; // { dg-error "attribute appertains" } > + friend __attribute__((deprecated)) int; // { dg-error "attribute appertains" } > + friend int __attribute__((deprecated)); // { dg-error "attribute appertains" } > + [[deprecated]] friend X; // { dg-error "attribute appertains" } > + [[deprecated]] friend class N; // { dg-warning "attribute ignored" } > + friend class [[deprecated]] N2; // { dg-error "attribute appertains" } > + friend class __attribute__((deprecated)) N3; // { dg-error "attribute appertains" } > + [[deprecated]] friend void foo<>(int, int); // { dg-error "attribute appertains" } > + // FIXME: Add dg error when PR100339 is resolved. > + //[[deprecated]] friend void ::foo(int, int); > +}; > + > +template > +class node { }; > + > +template > +struct A { > + [[deprecated]] friend T; // { dg-error "attribute appertains" } > + [[deprecated]] friend class node; // { dg-warning "attribute ignored" } > + template > + [[deprecated]] friend class A; // { dg-warning "attribute ignored" } > + template > + [[deprecated]] friend void bar () { } > + template > + [[deprecated]] friend void baz (); // { dg-error "attribute appertains" } > +}; > diff --git a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-39-1.C b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-39-1.C > index 453fc01a2e9..4010ba7724c 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-39-1.C > +++ b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-39-1.C > @@ -2,7 +2,8 @@ > > int fragile_block(void) { > typedef > - [[gnu::aligned (16)]] // { dg-warning "ignored" } > + [[gnu::aligned (16)]] // { dg-error "standard attributes in middle of decl-specifiers" } > +// { dg-warning "attribute ignored" "" { target *-*-* } .-1 } > struct { > int i; > } XmmUint16; > diff --git a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-4.C b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-4.C > index b401c6908e3..c120aeddf95 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-4.C > +++ b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-4.C > @@ -13,7 +13,8 @@ int one_third [[noreturn]] [[gnu::unused]] (void); > int [[gnu::unused]] one_half(); // { dg-warning "ignored" } > > static > -[[noreturn]] // { dg-warning "ignored" } > +[[noreturn]] // { dg-error "standard attributes in middle of decl-specifiers" } > +// { dg-warning "attribute ignored" "" { target *-*-* } .-1 } > void two [[gnu::unused]] (void) {} > > > diff --git a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-74.C b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-74.C > new file mode 100644 > index 00000000000..7e17bc8b661 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-74.C > @@ -0,0 +1,10 @@ > +// { dg-do compile { target c++11 } } > +// A C++11 attribute cannot appear in the middle of the decl-specifier-seq, > +// only before it (in which case it appertains to the declaration) or at > +// the end (in which case it appertains to the type). > + > +struct S { > + friend [[deprecated]] void; // { dg-error "standard attributes in middle of decl-specifiers" } > + friend [[deprecated]] int fn(); // { dg-error "standard attributes in middle of decl-specifiers" } > + // { dg-warning "attribute ignored" "" { target *-*-* } .-1 } > +}; > diff --git a/gcc/testsuite/g++.dg/ext/attrib63.C b/gcc/testsuite/g++.dg/ext/attrib63.C > new file mode 100644 > index 00000000000..783f80d0044 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/attrib63.C > @@ -0,0 +1,28 @@ > +// { dg-do compile } > + > +#define vector __attribute__((vector_size(16))) > +class A { > + friend vector float f(); > + __attribute__((deprecated)) friend void f2(); // { dg-error "attribute appertains" } > + friend __attribute__((deprecated, vector_size(16))) float f3(); > + friend __attribute__((vector_size(16), deprecated)) float f4(); > +}; > + > +vector float vf; > +vector float > +f () > +{ > + return vf; > +} > + > +vector float > +f3 () > +{ > + return vf; > +} > + > +vector float > +f4 () > +{ > + return vf; > +} > > base-commit: d21963ce7a87db3d4a6921a0fa98b72ea6f4e7f5 >