Message ID | ZcpVz7goxYjQLYwY@tucnak |
---|---|
State | New |
Headers | show |
Series | attribs: Don't canonicalize lookup_scoped_attribute_spec argument [PR113674] | expand |
On Mon, Feb 12, 2024 at 06:30:55PM +0100, Jakub Jelinek wrote: > Hi! > > The C and C++ FEs when parsing attributes already canonicalize them > (i.e. if they start with __ and end with __ substrings, we remove those). > lookup_attribute already verifies in gcc_assert that the first character > of name is not an underscore, and even lookup_scoped_attribute_spec doesn't > attempt to canonicalize the namespace it is passed. But for some historic > reason it was canonicalizing the name argument, which misbehaves when > an attribute starts with ____ and ends with ____. > I believe it is just wrong to try to canonicalize > lookup_scope_attribute_spec name attribute, it should have been > canonicalized already, in other spots where it is called it is already > canonicalized before. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Looks fine. extract_attribute_substring was added in https://gcc.gnu.org/pipermail/gcc-patches/2007-July/221563.html but with no tests. I see that without it gcc.dg/attr-noinline.c was failing, but that hasn't been the case for many years now. So I don't know why it would be necessary to keep it. Marek
On Mon, Feb 12, 2024 at 01:15:55PM -0500, Marek Polacek wrote: > On Mon, Feb 12, 2024 at 06:30:55PM +0100, Jakub Jelinek wrote: > > The C and C++ FEs when parsing attributes already canonicalize them > > (i.e. if they start with __ and end with __ substrings, we remove those). > > lookup_attribute already verifies in gcc_assert that the first character > > of name is not an underscore, and even lookup_scoped_attribute_spec doesn't > > attempt to canonicalize the namespace it is passed. But for some historic > > reason it was canonicalizing the name argument, which misbehaves when > > an attribute starts with ____ and ends with ____. > > I believe it is just wrong to try to canonicalize > > lookup_scope_attribute_spec name attribute, it should have been > > canonicalized already, in other spots where it is called it is already > > canonicalized before. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > Looks fine. extract_attribute_substring was added in > https://gcc.gnu.org/pipermail/gcc-patches/2007-July/221563.html > but with no tests. I see that without it gcc.dg/attr-noinline.c > was failing, but that hasn't been the case for many years now. So > I don't know why it would be necessary to keep it. I think it was added far before Martin's changes to canonicalize attributes in r8-2418. At that point attributes in the attributes lists weren't canonicalized, so it was necessary to canonicalize them before or during the lookups. Jakub
On 2/12/24 12:30, Jakub Jelinek wrote: > Hi! > > The C and C++ FEs when parsing attributes already canonicalize them > (i.e. if they start with __ and end with __ substrings, we remove those). > lookup_attribute already verifies in gcc_assert that the first character > of name is not an underscore, and even lookup_scoped_attribute_spec doesn't > attempt to canonicalize the namespace it is passed. But for some historic > reason it was canonicalizing the name argument, which misbehaves when > an attribute starts with ____ and ends with ____. > I believe it is just wrong to try to canonicalize > lookup_scope_attribute_spec name attribute, it should have been > canonicalized already, in other spots where it is called it is already > canonicalized before. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. > 2024-02-12 Jakub Jelinek <jakub@redhat.com> > > PR c++/113674 > * attribs.cc (extract_attribute_substring): Remove. > (lookup_scoped_attribute_spec): Don't call it. > > * c-c++-common/Wattributes-3.c: New test. > > --- gcc/attribs.cc.jj 2024-01-03 11:51:39.392622088 +0100 > +++ gcc/attribs.cc 2024-02-12 12:50:00.660458907 +0100 > @@ -116,15 +116,6 @@ get_gnu_namespace () > return gnu_namespace_cache; > } > > -/* Return base name of the attribute. Ie '__attr__' is turned into 'attr'. > - To avoid need for copying, we simply return length of the string. */ > - > -static void > -extract_attribute_substring (struct substring *str) > -{ > - canonicalize_attr_name (str->str, str->length); > -} > - > /* Insert SPECS into its namespace. IGNORED_P is true iff all unknown > attributes in this namespace should be ignored for the purposes of > -Wattributes. The function returns the namespace into which the > @@ -398,7 +389,6 @@ lookup_scoped_attribute_spec (const_tree > > attr.str = IDENTIFIER_POINTER (name); > attr.length = IDENTIFIER_LENGTH (name); > - extract_attribute_substring (&attr); > return attrs->attribute_hash->find_with_hash (&attr, > substring_hash (attr.str, > attr.length)); > --- gcc/testsuite/c-c++-common/Wattributes-3.c.jj 2024-02-12 13:04:21.964502985 +0100 > +++ gcc/testsuite/c-c++-common/Wattributes-3.c 2024-02-12 13:06:32.659688900 +0100 > @@ -0,0 +1,13 @@ > +/* PR c++/113674 */ > +/* { dg-do compile { target { c || c++11 } } } */ > +/* { dg-options "" } */ > + > +[[____noreturn____]] int foo (int i) /* { dg-warning "'__noreturn__' attribute (directive )?ignored" } */ > +{ > + return i; > +} > + > +[[____maybe_unused____]] int bar (int i) /* { dg-warning "'__maybe_unused__' attribute (directive )?ignored" } */ > +{ > + return i; > +} > > Jakub >
--- gcc/attribs.cc.jj 2024-01-03 11:51:39.392622088 +0100 +++ gcc/attribs.cc 2024-02-12 12:50:00.660458907 +0100 @@ -116,15 +116,6 @@ get_gnu_namespace () return gnu_namespace_cache; } -/* Return base name of the attribute. Ie '__attr__' is turned into 'attr'. - To avoid need for copying, we simply return length of the string. */ - -static void -extract_attribute_substring (struct substring *str) -{ - canonicalize_attr_name (str->str, str->length); -} - /* Insert SPECS into its namespace. IGNORED_P is true iff all unknown attributes in this namespace should be ignored for the purposes of -Wattributes. The function returns the namespace into which the @@ -398,7 +389,6 @@ lookup_scoped_attribute_spec (const_tree attr.str = IDENTIFIER_POINTER (name); attr.length = IDENTIFIER_LENGTH (name); - extract_attribute_substring (&attr); return attrs->attribute_hash->find_with_hash (&attr, substring_hash (attr.str, attr.length)); --- gcc/testsuite/c-c++-common/Wattributes-3.c.jj 2024-02-12 13:04:21.964502985 +0100 +++ gcc/testsuite/c-c++-common/Wattributes-3.c 2024-02-12 13:06:32.659688900 +0100 @@ -0,0 +1,13 @@ +/* PR c++/113674 */ +/* { dg-do compile { target { c || c++11 } } } */ +/* { dg-options "" } */ + +[[____noreturn____]] int foo (int i) /* { dg-warning "'__noreturn__' attribute (directive )?ignored" } */ +{ + return i; +} + +[[____maybe_unused____]] int bar (int i) /* { dg-warning "'__maybe_unused__' attribute (directive )?ignored" } */ +{ + return i; +}