Re: [PATCH] attribs: Don't canonicalize lookup_scoped_attribute_spec argument [PR113674]
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
Re: [PATCH] attribs: Don't canonicalize lookup_scoped_attribute_spec argument [PR113674]
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
Re: [PATCH] attribs: Don't canonicalize lookup_scoped_attribute_spec argument [PR113674]
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
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
