From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 63649 invoked by alias); 16 Jun 2019 16:37:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 63635 invoked by uid 89); 16 Jun 2019 16:37:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= 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 ESMTP; Sun, 16 Jun 2019 16:37:16 +0000 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C0B1F3689B for ; Sun, 16 Jun 2019 16:37:00 +0000 (UTC) Received: from redhat.com (ovpn-120-65.rdu2.redhat.com [10.10.120.65]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 15F141001284; Sun, 16 Jun 2019 16:36:59 +0000 (UTC) Date: Sun, 16 Jun 2019 16:37:00 -0000 From: Marek Polacek To: Jakub Jelinek Cc: GCC Patches , Jason Merrill Subject: Re: C++ PATCH for c++/60364 - noreturn after first decl not diagnosed (v3) Message-ID: <20190616163658.GL5989@redhat.com> References: <20190615142917.GI5989@redhat.com> <20190615143326.GK19695@tucnak> <20190615143913.GJ5989@redhat.com> <20190616161037.GK5989@redhat.com> <20190616161856.GL19695@tucnak> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190616161856.GL19695@tucnak> User-Agent: Mutt/1.11.4 (2019-03-13) X-SW-Source: 2019-06/txt/msg00895.txt.bz2 On Sun, Jun 16, 2019 at 06:18:56PM +0200, Jakub Jelinek wrote: > On Sun, Jun 16, 2019 at 12:10:37PM -0400, Marek Polacek wrote: > > > Guess I will really have to make the changes to treat [[noreturn]] similarly > > > to e.g. [[nodiscard]], so that cxx11_attribute_p works. > > > > Thus. Changes I've made: > > * don't treat [[noreturn]] as an equivalent to __attribute__((noreturn)); > > * for that I had to adjust decl_attributes, it wasn't preserving the > > C++11 form (a list in another list); fix shadowing while at it; > > * the above turned up two spots that were wrongly accessing TREE_PURPOSE > > directly instead of using get_attribute_name; > > * give error only for [[noreturn]] but not for __attribute__((noreturn)) > > or [[gnu::noreturn]]. > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > I'd prefer to defer review of this to Jason, just want to note that I don't > see any testsuite coverage on mixing declarations with different forms of > attributes ([[noreturn]] on one decl and __attribute__((noreturn)) or > [[gnu::noreturn]] on another one or vice versa. Added now. I suppose it should compile fine, which it does. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2019-06-16 Marek Polacek PR c++/60364 - noreturn after first decl not diagnosed. * attribs.c (get_attribute_namespace): No longer static. (decl_attributes): Avoid shadowing. Preserve the C++11 form for C++11 attributes. * attribs.h (get_attribute_namespace): Declare. * tree-inline.c (function_attribute_inlinable_p): Use get_attribute_name. * c-attribs.c (handle_noreturn_attribute): No longer static. * c-common.h (handle_noreturn_attribute): Declare. * c-format.c (check_function_format): Use get_attribute_name. * decl.c (duplicate_decls): Give an error when a function is declared [[noreturn]] after its first declaration. * parser.c (cp_parser_std_attribute): Don't treat C++11 noreturn attribute as equivalent to GNU's. * tree.c (std_attribute_table): Add noreturn. * g++.dg/warn/noreturn-8.C: New test. * g++.dg/warn/noreturn-9.C: New test. diff --git gcc/attribs.c gcc/attribs.c index 4441922543f..8e540165597 100644 --- gcc/attribs.c +++ gcc/attribs.c @@ -340,7 +340,7 @@ lookup_attribute_spec (const_tree name) Please read the comments of cxx11_attribute_p to understand the format of attributes. */ -static tree +tree get_attribute_namespace (const_tree attr) { if (cxx11_attribute_p (attr)) @@ -469,7 +469,6 @@ tree decl_attributes (tree *node, tree attributes, int flags, tree last_decl /* = NULL_TREE */) { - tree a; tree returned_attrs = NULL_TREE; if (TREE_TYPE (*node) == error_mark_node || attributes == error_mark_node) @@ -548,22 +547,23 @@ decl_attributes (tree *node, tree attributes, int flags, /* Note that attributes on the same declaration are not necessarily in the same order as in the source. */ - for (a = attributes; a; a = TREE_CHAIN (a)) + for (tree attr = attributes; attr; attr = TREE_CHAIN (attr)) { - tree ns = get_attribute_namespace (a); - tree name = get_attribute_name (a); - tree args = TREE_VALUE (a); + tree ns = get_attribute_namespace (attr); + tree name = get_attribute_name (attr); + tree args = TREE_VALUE (attr); tree *anode = node; const struct attribute_spec *spec = lookup_scoped_attribute_spec (ns, name); int fn_ptr_quals = 0; tree fn_ptr_tmp = NULL_TREE; + const bool cxx11_attr_p = cxx11_attribute_p (attr); if (spec == NULL) { if (!(flags & (int) ATTR_FLAG_BUILT_IN)) { - if (ns == NULL_TREE || !cxx11_attribute_p (a)) + if (ns == NULL_TREE || !cxx11_attr_p) warning (OPT_Wattributes, "%qE attribute directive ignored", name); else @@ -584,7 +584,7 @@ decl_attributes (tree *node, tree attributes, int flags, gcc_assert (is_attribute_p (spec->name, name)); if (TYPE_P (*node) - && cxx11_attribute_p (a) + && cxx11_attr_p && !(flags & ATTR_FLAG_TYPE_IN_PLACE)) { /* This is a c++11 attribute that appertains to a @@ -707,8 +707,7 @@ decl_attributes (tree *node, tree attributes, int flags, if (spec->handler != NULL) { - int cxx11_flag = - cxx11_attribute_p (a) ? ATTR_FLAG_CXX11 : 0; + int cxx11_flag = (cxx11_attr_p ? ATTR_FLAG_CXX11 : 0); /* Pass in an array of the current declaration followed by the last pushed/merged declaration if one exists. @@ -756,17 +755,23 @@ decl_attributes (tree *node, tree attributes, int flags, if (a == NULL_TREE) { /* This attribute isn't already in the list. */ + tree r; + /* Preserve the C++11 form. */ + if (cxx11_attr_p) + r = tree_cons (build_tree_list (ns, name), args, old_attrs); + else + r = tree_cons (name, args, old_attrs); + if (DECL_P (*anode)) - DECL_ATTRIBUTES (*anode) = tree_cons (name, args, old_attrs); + DECL_ATTRIBUTES (*anode) = r; else if (flags & (int) ATTR_FLAG_TYPE_IN_PLACE) { - TYPE_ATTRIBUTES (*anode) = tree_cons (name, args, old_attrs); + TYPE_ATTRIBUTES (*anode) = r; /* If this is the main variant, also push the attributes out to the other variants. */ if (*anode == TYPE_MAIN_VARIANT (*anode)) { - tree variant; - for (variant = *anode; variant; + for (tree variant = *anode; variant; variant = TYPE_NEXT_VARIANT (variant)) { if (TYPE_ATTRIBUTES (variant) == old_attrs) @@ -780,9 +785,7 @@ decl_attributes (tree *node, tree attributes, int flags, } } else - *anode = build_type_attribute_variant (*anode, - tree_cons (name, args, - old_attrs)); + *anode = build_type_attribute_variant (*anode, r); } } diff --git gcc/attribs.h gcc/attribs.h index 83ecbbbeec9..23a7321e04a 100644 --- gcc/attribs.h +++ gcc/attribs.h @@ -35,6 +35,7 @@ extern tree decl_attributes (tree *, tree, int, tree = NULL_TREE); extern bool cxx11_attribute_p (const_tree); extern tree get_attribute_name (const_tree); +extern tree get_attribute_namespace (const_tree); extern void apply_tm_attr (tree, tree); extern tree make_attribute (const char *, const char *, tree); diff --git gcc/c-family/c-attribs.c gcc/c-family/c-attribs.c index 7a8d3935d79..1cd95dc6665 100644 --- gcc/c-family/c-attribs.c +++ gcc/c-family/c-attribs.c @@ -49,7 +49,6 @@ along with GCC; see the file COPYING3. If not see static tree handle_packed_attribute (tree *, tree, tree, int, bool *); static tree handle_nocommon_attribute (tree *, tree, tree, int, bool *); static tree handle_common_attribute (tree *, tree, tree, int, bool *); -static tree handle_noreturn_attribute (tree *, tree, tree, int, bool *); static tree handle_hot_attribute (tree *, tree, tree, int, bool *); static tree handle_cold_attribute (tree *, tree, tree, int, bool *); static tree handle_no_sanitize_attribute (tree *, tree, tree, int, bool *); @@ -779,7 +778,7 @@ handle_common_attribute (tree *node, tree name, tree ARG_UNUSED (args), /* Handle a "noreturn" attribute; arguments as in struct attribute_spec.handler. */ -static tree +tree handle_noreturn_attribute (tree *node, tree name, tree ARG_UNUSED (args), int ARG_UNUSED (flags), bool *no_add_attrs) { diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h index 1cf2cae6395..079225ca127 100644 --- gcc/c-family/c-common.h +++ gcc/c-family/c-common.h @@ -1344,6 +1344,7 @@ extern int tm_attr_to_mask (tree); extern tree tm_mask_to_attr (int); extern tree find_tm_attribute (tree); extern const struct attribute_spec::exclusions attr_cold_hot_exclusions[]; +extern tree handle_noreturn_attribute (tree *, tree, tree, int, bool *); /* In c-format.c. */ extern bool valid_format_string_type_p (tree); diff --git gcc/c-family/c-format.c gcc/c-family/c-format.c index b10599f9982..bd9d5aeda4e 100644 --- gcc/c-family/c-format.c +++ gcc/c-family/c-format.c @@ -1110,7 +1110,7 @@ check_function_format (const_tree fntype, tree attrs, int nargs, /* See if this function has any format attributes. */ for (a = attrs; a; a = TREE_CHAIN (a)) { - if (is_attribute_p ("format", TREE_PURPOSE (a))) + if (is_attribute_p ("format", get_attribute_name (a))) { /* Yup; check it. */ function_format_info info; diff --git gcc/cp/decl.c gcc/cp/decl.c index 0a3ef452536..85f96f73739 100644 --- gcc/cp/decl.c +++ gcc/cp/decl.c @@ -1922,10 +1922,29 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend) if (TREE_CODE (newdecl) == FUNCTION_DECL) { - if (merge_attr && diagnose_mismatched_attributes (olddecl, newdecl)) - inform (olddecl_loc, DECL_INITIAL (olddecl) - ? G_("previous definition of %qD here") - : G_("previous declaration of %qD here"), olddecl); + if (merge_attr) + { + if (diagnose_mismatched_attributes (olddecl, newdecl)) + inform (olddecl_loc, DECL_INITIAL (olddecl) + ? G_("previous definition of %qD here") + : G_("previous declaration of %qD here"), olddecl); + + /* [dcl.attr.noreturn]: The first declaration of a function shall + specify the noreturn attribute if any declaration of that function + specifies the noreturn attribute. */ + tree a; + if (TREE_THIS_VOLATILE (newdecl) + && !TREE_THIS_VOLATILE (olddecl) + /* This applies to [[noreturn]] only, not its GNU variants. */ + && (a = lookup_attribute ("noreturn", DECL_ATTRIBUTES (newdecl))) + && cxx11_attribute_p (a) + && get_attribute_namespace (a) == NULL_TREE) + { + error_at (newdecl_loc, "function %qD declared %<[[noreturn]]%> " + "but its first declaration was not", newdecl); + inform (olddecl_loc, "previous declaration of %qD", olddecl); + } + } /* Now that functions must hold information normally held by field decls, there is extra work to do so that diff --git gcc/cp/parser.c gcc/cp/parser.c index 8f5ae84670a..996e54235fd 100644 --- gcc/cp/parser.c +++ gcc/cp/parser.c @@ -26121,11 +26121,11 @@ cp_parser_std_attribute (cp_parser *parser, tree attr_ns) attr_id = canonicalize_attr_name (attr_id); attribute = build_tree_list (build_tree_list (NULL_TREE, attr_id), NULL_TREE); - /* C++11 noreturn attribute is equivalent to GNU's. */ - if (is_attribute_p ("noreturn", attr_id)) - TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier; + /* We used to treat C++11 noreturn attribute as equivalent to GNU's, + but no longer: we have to be able to tell [[noreturn]] and + __attribute__((noreturn)) apart. */ /* C++14 deprecated attribute is equivalent to GNU's. */ - else if (is_attribute_p ("deprecated", attr_id)) + if (is_attribute_p ("deprecated", attr_id)) TREE_PURPOSE (TREE_PURPOSE (attribute)) = gnu_identifier; /* C++17 fallthrough attribute is equivalent to GNU's. */ else if (is_attribute_p ("fallthrough", attr_id)) diff --git gcc/cp/tree.c gcc/cp/tree.c index cd021b7f594..bb695e14e73 100644 --- gcc/cp/tree.c +++ gcc/cp/tree.c @@ -4453,6 +4453,8 @@ const struct attribute_spec std_attribute_table[] = handle_likeliness_attribute, attr_cold_hot_exclusions }, { "unlikely", 0, 0, false, false, false, false, handle_likeliness_attribute, attr_cold_hot_exclusions }, + { "noreturn", 0, 0, true, false, false, false, + handle_noreturn_attribute, NULL }, { NULL, 0, 0, false, false, false, false, NULL, NULL } }; diff --git gcc/testsuite/g++.dg/warn/noreturn-8.C gcc/testsuite/g++.dg/warn/noreturn-8.C new file mode 100644 index 00000000000..d465468decb --- /dev/null +++ gcc/testsuite/g++.dg/warn/noreturn-8.C @@ -0,0 +1,21 @@ +// PR c++/60364 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpedantic" } + +void f (); // { dg-message "previous declaration" } +void f [[noreturn]] (); // { dg-error "declared '\\\[\\\[noreturn\\\]\\\]' but its first declaration was not" } + +void f2 (); +void f2 [[gnu::noreturn]] (); + +void f3 (); +__attribute__((noreturn)) void f3 (); + +void f4 () { __builtin_abort (); } // { dg-message "previous declaration" } +void f4 [[noreturn]] (); // { dg-error "declared '\\\[\\\[noreturn\\\]\\\]' but its first declaration was not" } + +void f5 () { __builtin_abort (); } +void f5 [[gnu::noreturn]] (); + +void f6 () { __builtin_abort (); } +__attribute__((noreturn)) void f6 (); diff --git gcc/testsuite/g++.dg/warn/noreturn-9.C gcc/testsuite/g++.dg/warn/noreturn-9.C new file mode 100644 index 00000000000..f7ede57aab0 --- /dev/null +++ gcc/testsuite/g++.dg/warn/noreturn-9.C @@ -0,0 +1,21 @@ +// PR c++/60364 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpedantic" } + +void f1 [[gnu::noreturn]] (); +void f1 [[noreturn]] (); + +void f2 [[noreturn]] (); +void f2 [[gnu::noreturn]] (); + +__attribute__((noreturn)) void f3 (); +void f3 [[noreturn]] (); + +void f4 [[noreturn]] (); +__attribute__((noreturn)) void f4 (); + +__attribute__((noreturn)) void f5 (); +void f5 [[gnu::noreturn]] (); + +void f6 [[gnu::noreturn]] (); +__attribute__((noreturn)) void f6 (); diff --git gcc/tree-inline.c gcc/tree-inline.c index 52f45a73b1d..2de5e22f10f 100644 --- gcc/tree-inline.c +++ gcc/tree-inline.c @@ -3899,7 +3899,7 @@ function_attribute_inlinable_p (const_tree fndecl) for (a = DECL_ATTRIBUTES (fndecl); a; a = TREE_CHAIN (a)) { - const_tree name = TREE_PURPOSE (a); + const_tree name = get_attribute_name (a); int i; for (i = 0; targetm.attribute_table[i].name != NULL; i++)