* [PATCH] c++: Fix C++11 attribute propagation [PR106712] @ 2022-08-26 23:01 Marek Polacek 2022-08-29 17:32 ` Jason Merrill 0 siblings, 1 reply; 4+ messages in thread From: Marek Polacek @ 2022-08-26 23:01 UTC (permalink / raw) To: Jason Merrill, GCC Patches When we have [[noreturn]] int fn1 [[nodiscard]](), fn2(); "noreturn" should apply to both fn1 and fn2 but "nodiscard" only to fn1: [dcl.pre]/3: "The attribute-specifier-seq appertains to each of the entities declared by the declarators of the init-declarator-list." [dcl.spec.general]: "The attribute-specifier-seq affects the type only for the declaration it appears in, not other declarations involving the same type." As Ed Catmur correctly analyzed, this is because, for the test above, we call start_decl with prefix_attributes=noreturn, but this line: attributes = attr_chainon (attributes, prefix_attributes); results in attributes == prefix_attributes, because chainon sees that attributes is null so it just returns prefix_attributes. Then in grokdeclarator we reach *attrlist = attr_chainon (*attrlist, declarator->std_attributes); which modifies prefix_attributes so now it's "noreturn, nodiscard" and so fn2 is wrongly marked nodiscard as well. Fixed by copying prefix_attributes. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? PR c++/106712 gcc/cp/ChangeLog: * decl.cc (start_decl): Copy prefix_attributes. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/gen-attrs-77.C: New test. --- gcc/cp/decl.cc | 3 ++- gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index d46a347a6c7..9fa80b926d5 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -5566,7 +5566,8 @@ start_decl (const cp_declarator *declarator, *pushed_scope_p = NULL_TREE; if (prefix_attributes != error_mark_node) - attributes = attr_chainon (attributes, prefix_attributes); + /* Don't let grokdeclarator modify prefix_attributes. */ + attributes = attr_chainon (attributes, copy_list (prefix_attributes)); decl = grokdeclarator (declarator, declspecs, NORMAL, initialized, &attributes); diff --git a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C new file mode 100644 index 00000000000..2c41c62f33b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C @@ -0,0 +1,17 @@ +// PR c++/106712 +// { dg-do compile { target c++11 } } + +[[noreturn]] int f1 [[nodiscard]](), f2 (); +[[nodiscard]] int f3 (), f4 (); +int f5 [[nodiscard]](), f6 (); + +int +main () +{ + f1 (); // { dg-warning "ignoring" } + f2 (); + f3 (); // { dg-warning "ignoring" } + f4 (); // { dg-warning "ignoring" } + f5 (); // { dg-warning "ignoring" } + f6 (); +} base-commit: 390f94eee1ae694901f896ac45bfb148f8126baa -- 2.37.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: Fix C++11 attribute propagation [PR106712] 2022-08-26 23:01 [PATCH] c++: Fix C++11 attribute propagation [PR106712] Marek Polacek @ 2022-08-29 17:32 ` Jason Merrill 2022-08-29 20:01 ` [PATCH v2] " Marek Polacek 0 siblings, 1 reply; 4+ messages in thread From: Jason Merrill @ 2022-08-29 17:32 UTC (permalink / raw) To: Marek Polacek, GCC Patches On 8/26/22 19:01, Marek Polacek wrote: > When we have > > [[noreturn]] int fn1 [[nodiscard]](), fn2(); > > "noreturn" should apply to both fn1 and fn2 but "nodiscard" only to fn1: > [dcl.pre]/3: "The attribute-specifier-seq appertains to each of > the entities declared by the declarators of the init-declarator-list." > [dcl.spec.general]: "The attribute-specifier-seq affects the type > only for the declaration it appears in, not other declarations involving > the same type." > > As Ed Catmur correctly analyzed, this is because, for the test above, > we call start_decl with prefix_attributes=noreturn, but this line: > > attributes = attr_chainon (attributes, prefix_attributes); > > results in attributes == prefix_attributes, because chainon sees > that attributes is null so it just returns prefix_attributes. Then > in grokdeclarator we reach > > *attrlist = attr_chainon (*attrlist, declarator->std_attributes); > > which modifies prefix_attributes so now it's "noreturn, nodiscard" > and so fn2 is wrongly marked nodiscard as well. Fixed by copying > prefix_attributes. How about reversing the order of arguments to the call in grokdeclarator, so that the prefix attributes can remain shared at the end of the list? > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/106712 > > gcc/cp/ChangeLog: > > * decl.cc (start_decl): Copy prefix_attributes. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/gen-attrs-77.C: New test. > --- > gcc/cp/decl.cc | 3 ++- > gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C | 17 +++++++++++++++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index d46a347a6c7..9fa80b926d5 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -5566,7 +5566,8 @@ start_decl (const cp_declarator *declarator, > *pushed_scope_p = NULL_TREE; > > if (prefix_attributes != error_mark_node) > - attributes = attr_chainon (attributes, prefix_attributes); > + /* Don't let grokdeclarator modify prefix_attributes. */ > + attributes = attr_chainon (attributes, copy_list (prefix_attributes)); > > decl = grokdeclarator (declarator, declspecs, NORMAL, initialized, > &attributes); > diff --git a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C > new file mode 100644 > index 00000000000..2c41c62f33b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C > @@ -0,0 +1,17 @@ > +// PR c++/106712 > +// { dg-do compile { target c++11 } } > + > +[[noreturn]] int f1 [[nodiscard]](), f2 (); > +[[nodiscard]] int f3 (), f4 (); > +int f5 [[nodiscard]](), f6 (); > + > +int > +main () > +{ > + f1 (); // { dg-warning "ignoring" } > + f2 (); > + f3 (); // { dg-warning "ignoring" } > + f4 (); // { dg-warning "ignoring" } > + f5 (); // { dg-warning "ignoring" } > + f6 (); > +} > > base-commit: 390f94eee1ae694901f896ac45bfb148f8126baa ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] c++: Fix C++11 attribute propagation [PR106712] 2022-08-29 17:32 ` Jason Merrill @ 2022-08-29 20:01 ` Marek Polacek 2022-08-29 21:16 ` Jason Merrill 0 siblings, 1 reply; 4+ messages in thread From: Marek Polacek @ 2022-08-29 20:01 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches On Mon, Aug 29, 2022 at 01:32:29PM -0400, Jason Merrill wrote: > On 8/26/22 19:01, Marek Polacek wrote: > > When we have > > > > [[noreturn]] int fn1 [[nodiscard]](), fn2(); > > > > "noreturn" should apply to both fn1 and fn2 but "nodiscard" only to fn1: > > [dcl.pre]/3: "The attribute-specifier-seq appertains to each of > > the entities declared by the declarators of the init-declarator-list." > > [dcl.spec.general]: "The attribute-specifier-seq affects the type > > only for the declaration it appears in, not other declarations involving > > the same type." > > > > As Ed Catmur correctly analyzed, this is because, for the test above, > > we call start_decl with prefix_attributes=noreturn, but this line: > > > > attributes = attr_chainon (attributes, prefix_attributes); > > > > results in attributes == prefix_attributes, because chainon sees > > that attributes is null so it just returns prefix_attributes. Then > > in grokdeclarator we reach > > > > *attrlist = attr_chainon (*attrlist, declarator->std_attributes); > > > > which modifies prefix_attributes so now it's "noreturn, nodiscard" > > and so fn2 is wrongly marked nodiscard as well. Fixed by copying > > prefix_attributes. > > How about reversing the order of arguments to the call in grokdeclarator, so > that the prefix attributes can remain shared at the end of the list? Thanks, that seems like a cheaper solution. It works because this way we tack the prefix attributes onto ->std_attributes, avoiding modifying prefix_attributes. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- When we have [[noreturn]] int fn1 [[nodiscard]](), fn2(); "noreturn" should apply to both fn1 and fn2 but "nodiscard" only to fn1: [dcl.pre]/3: "The attribute-specifier-seq appertains to each of the entities declared by the declarators of the init-declarator-list." [dcl.spec.general]: "The attribute-specifier-seq affects the type only for the declaration it appears in, not other declarations involving the same type." As Ed Catmur correctly analyzed, this is because, for the test above, we call start_decl with prefix_attributes=noreturn, but this line: attributes = attr_chainon (attributes, prefix_attributes); results in attributes == prefix_attributes, because chainon sees that attributes is null so it just returns prefix_attributes. Then in grokdeclarator we reach *attrlist = attr_chainon (*attrlist, declarator->std_attributes); which modifies prefix_attributes so now it's "noreturn, nodiscard" and so fn2 is wrongly marked nodiscard as well. Fixed by reversing the order of arguments to attr_chainon. That way, we tack the prefix attributes onto ->std_attributes, avoiding modifying prefix_attributes. PR c++/106712 gcc/cp/ChangeLog: * decl.cc (grokdeclarator): Reverse the order of arguments to attr_chainon. gcc/testsuite/ChangeLog: * g++.dg/cpp0x/gen-attrs-77.C: New test. --- gcc/cp/decl.cc | 2 +- gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc index d46a347a6c7..b72b2a8456b 100644 --- a/gcc/cp/decl.cc +++ b/gcc/cp/decl.cc @@ -13474,7 +13474,7 @@ grokdeclarator (const cp_declarator *declarator, /* [dcl.meaning]/1: The optional attribute-specifier-seq following a declarator-id appertains to the entity that is declared. */ if (declarator->std_attributes != error_mark_node) - *attrlist = attr_chainon (*attrlist, declarator->std_attributes); + *attrlist = attr_chainon (declarator->std_attributes, *attrlist); else /* We should have already diagnosed the issue (c++/78344). */ gcc_assert (seen_error ()); diff --git a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C new file mode 100644 index 00000000000..2c41c62f33b --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C @@ -0,0 +1,17 @@ +// PR c++/106712 +// { dg-do compile { target c++11 } } + +[[noreturn]] int f1 [[nodiscard]](), f2 (); +[[nodiscard]] int f3 (), f4 (); +int f5 [[nodiscard]](), f6 (); + +int +main () +{ + f1 (); // { dg-warning "ignoring" } + f2 (); + f3 (); // { dg-warning "ignoring" } + f4 (); // { dg-warning "ignoring" } + f5 (); // { dg-warning "ignoring" } + f6 (); +} base-commit: 60d1d296b42b22b08d4eaa38bea02100c07261ac -- 2.37.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] c++: Fix C++11 attribute propagation [PR106712] 2022-08-29 20:01 ` [PATCH v2] " Marek Polacek @ 2022-08-29 21:16 ` Jason Merrill 0 siblings, 0 replies; 4+ messages in thread From: Jason Merrill @ 2022-08-29 21:16 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches On 8/29/22 16:01, Marek Polacek wrote: > On Mon, Aug 29, 2022 at 01:32:29PM -0400, Jason Merrill wrote: >> On 8/26/22 19:01, Marek Polacek wrote: >>> When we have >>> >>> [[noreturn]] int fn1 [[nodiscard]](), fn2(); >>> >>> "noreturn" should apply to both fn1 and fn2 but "nodiscard" only to fn1: >>> [dcl.pre]/3: "The attribute-specifier-seq appertains to each of >>> the entities declared by the declarators of the init-declarator-list." >>> [dcl.spec.general]: "The attribute-specifier-seq affects the type >>> only for the declaration it appears in, not other declarations involving >>> the same type." >>> >>> As Ed Catmur correctly analyzed, this is because, for the test above, >>> we call start_decl with prefix_attributes=noreturn, but this line: >>> >>> attributes = attr_chainon (attributes, prefix_attributes); >>> >>> results in attributes == prefix_attributes, because chainon sees >>> that attributes is null so it just returns prefix_attributes. Then >>> in grokdeclarator we reach >>> >>> *attrlist = attr_chainon (*attrlist, declarator->std_attributes); >>> >>> which modifies prefix_attributes so now it's "noreturn, nodiscard" >>> and so fn2 is wrongly marked nodiscard as well. Fixed by copying >>> prefix_attributes. >> >> How about reversing the order of arguments to the call in grokdeclarator, so >> that the prefix attributes can remain shared at the end of the list? > > Thanks, that seems like a cheaper solution. It works because this way > we tack the prefix attributes onto ->std_attributes, avoiding modifying > prefix_attributes. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK. > -- >8 -- > When we have > > [[noreturn]] int fn1 [[nodiscard]](), fn2(); > > "noreturn" should apply to both fn1 and fn2 but "nodiscard" only to fn1: > [dcl.pre]/3: "The attribute-specifier-seq appertains to each of > the entities declared by the declarators of the init-declarator-list." > [dcl.spec.general]: "The attribute-specifier-seq affects the type > only for the declaration it appears in, not other declarations involving > the same type." > > As Ed Catmur correctly analyzed, this is because, for the test above, > we call start_decl with prefix_attributes=noreturn, but this line: > > attributes = attr_chainon (attributes, prefix_attributes); > > results in attributes == prefix_attributes, because chainon sees > that attributes is null so it just returns prefix_attributes. Then > in grokdeclarator we reach > > *attrlist = attr_chainon (*attrlist, declarator->std_attributes); > > which modifies prefix_attributes so now it's "noreturn, nodiscard" > and so fn2 is wrongly marked nodiscard as well. Fixed by reversing > the order of arguments to attr_chainon. That way, we tack the prefix > attributes onto ->std_attributes, avoiding modifying prefix_attributes. > > PR c++/106712 > > gcc/cp/ChangeLog: > > * decl.cc (grokdeclarator): Reverse the order of arguments to > attr_chainon. > > gcc/testsuite/ChangeLog: > > * g++.dg/cpp0x/gen-attrs-77.C: New test. > --- > gcc/cp/decl.cc | 2 +- > gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C | 17 +++++++++++++++++ > 2 files changed, 18 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C > > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc > index d46a347a6c7..b72b2a8456b 100644 > --- a/gcc/cp/decl.cc > +++ b/gcc/cp/decl.cc > @@ -13474,7 +13474,7 @@ grokdeclarator (const cp_declarator *declarator, > /* [dcl.meaning]/1: The optional attribute-specifier-seq following > a declarator-id appertains to the entity that is declared. */ > if (declarator->std_attributes != error_mark_node) > - *attrlist = attr_chainon (*attrlist, declarator->std_attributes); > + *attrlist = attr_chainon (declarator->std_attributes, *attrlist); > else > /* We should have already diagnosed the issue (c++/78344). */ > gcc_assert (seen_error ()); > diff --git a/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C > new file mode 100644 > index 00000000000..2c41c62f33b > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/gen-attrs-77.C > @@ -0,0 +1,17 @@ > +// PR c++/106712 > +// { dg-do compile { target c++11 } } > + > +[[noreturn]] int f1 [[nodiscard]](), f2 (); > +[[nodiscard]] int f3 (), f4 (); > +int f5 [[nodiscard]](), f6 (); > + > +int > +main () > +{ > + f1 (); // { dg-warning "ignoring" } > + f2 (); > + f3 (); // { dg-warning "ignoring" } > + f4 (); // { dg-warning "ignoring" } > + f5 (); // { dg-warning "ignoring" } > + f6 (); > +} > > base-commit: 60d1d296b42b22b08d4eaa38bea02100c07261ac ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-29 21:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-26 23:01 [PATCH] c++: Fix C++11 attribute propagation [PR106712] Marek Polacek 2022-08-29 17:32 ` Jason Merrill 2022-08-29 20:01 ` [PATCH v2] " Marek Polacek 2022-08-29 21:16 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).