diff mbox series

attribs: Don't canonicalize lookup_scoped_attribute_spec argument [PR113674]

Message ID ZcpVz7goxYjQLYwY@tucnak
State Unresolved
Headers
Series attribs: Don't canonicalize lookup_scoped_attribute_spec argument [PR113674] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Feb. 12, 2024, 5:30 p.m. UTC 
  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?

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.


	Jakub

Comments

Marek Polacek Feb. 12, 2024, 6:15 p.m. UTC | #1 
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
Jakub Jelinek Feb. 12, 2024, 6:22 p.m. UTC | #2 
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
Jason Merrill Feb. 12, 2024, 7:22 p.m. UTC | #3 
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
>
diff mbox series

Patch

--- 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;
+}