attribs: Fix ICEs on attributes starting with _ [PR103365]
Commit Message
Hi!
As the patch shows, we have quite a few asserts that we don't call
lookup_attribute etc. with attr_name that starts with an underscore,
to make sure nobody is trying to call it with non-canonicalized
attribute name like "__cold__" instead of "cold".
We canonicalize only attributes that start with 2 underscores and end
with 2 underscores though.
Before Marek's patch, that wasn't an issue, we had no attributes like
"_foo" or "__bar_" etc., so lookup_scoped_attribute_spec would
always return NULL for those and we wouldn't try to register them,
look them up etc., just with -Wattributes would warn about them.
But now, as the new testcase shows, users can actually request such
attributes to be ignored, and we ICE for those during
register_scoped_attribute and when that is fixed, ICE later on when
somebody uses those attributes because they will be looked up
to find out that they should be ignored.
So, the following patch instead of or in addition to, depending on
how performance sensitive a particular spot is, checking that
attribute doesn't start with underscore allows attribute
names that start with underscore as long as it doesn't canonicalize
(i.e. doesn't start and end with 2 underscores).
In addition to that, I've noticed lookup_attribute_by_prefix
was calling get_attribute_name twice unnecessarily, and 2 tests
were running in c++98 mode with -std=c++98 -std=c++11 which IMHO
isn't useful because -std=c++11 testing is done too when testing
all language versions.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2021-11-24 Jakub Jelinek <jakub@redhat.com>
PR middle-end/103365
* attribs.h (lookup_attribute): Allow attr_name to start with
underscore, as long as canonicalize_attr_name returns false.
(lookup_attribute_by_prefix): Don't call get_attribute_name twice.
* attribs.c (extract_attribute_substring): Reimplement using
canonicalize_attr_name.
(register_scoped_attribute): Change gcc_assert into
gcc_checking_assert, verify !canonicalize_attr_name rather than
that str.str doesn't start with '_'.
* c-c++-common/Wno-attributes-1.c: Require effective target
c || c++11 and drop dg-additional-options.
* c-c++-common/Wno-attributes-2.c: Likewise.
* c-c++-common/Wno-attributes-4.c: New test.
Jakub
Comments
On Wed, Nov 24, 2021 at 12:48 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi!
>
> As the patch shows, we have quite a few asserts that we don't call
> lookup_attribute etc. with attr_name that starts with an underscore,
> to make sure nobody is trying to call it with non-canonicalized
> attribute name like "__cold__" instead of "cold".
> We canonicalize only attributes that start with 2 underscores and end
> with 2 underscores though.
> Before Marek's patch, that wasn't an issue, we had no attributes like
> "_foo" or "__bar_" etc., so lookup_scoped_attribute_spec would
> always return NULL for those and we wouldn't try to register them,
> look them up etc., just with -Wattributes would warn about them.
> But now, as the new testcase shows, users can actually request such
> attributes to be ignored, and we ICE for those during
> register_scoped_attribute and when that is fixed, ICE later on when
> somebody uses those attributes because they will be looked up
> to find out that they should be ignored.
>
> So, the following patch instead of or in addition to, depending on
> how performance sensitive a particular spot is, checking that
> attribute doesn't start with underscore allows attribute
> names that start with underscore as long as it doesn't canonicalize
> (i.e. doesn't start and end with 2 underscores).
> In addition to that, I've noticed lookup_attribute_by_prefix
> was calling get_attribute_name twice unnecessarily, and 2 tests
> were running in c++98 mode with -std=c++98 -std=c++11 which IMHO
> isn't useful because -std=c++11 testing is done too when testing
> all language versions.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2021-11-24 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/103365
> * attribs.h (lookup_attribute): Allow attr_name to start with
> underscore, as long as canonicalize_attr_name returns false.
> (lookup_attribute_by_prefix): Don't call get_attribute_name twice.
> * attribs.c (extract_attribute_substring): Reimplement using
> canonicalize_attr_name.
> (register_scoped_attribute): Change gcc_assert into
> gcc_checking_assert, verify !canonicalize_attr_name rather than
> that str.str doesn't start with '_'.
>
> * c-c++-common/Wno-attributes-1.c: Require effective target
> c || c++11 and drop dg-additional-options.
> * c-c++-common/Wno-attributes-2.c: Likewise.
> * c-c++-common/Wno-attributes-4.c: New test.
Only one comment on the new testcases, you might want to add a
testcase for the option on the command line too.
Thanks,
Andrew Pinski
>
> --- gcc/attribs.h.jj 2021-11-22 10:06:42.173498383 +0100
> +++ gcc/attribs.h 2021-11-23 23:35:13.757972934 +0100
> @@ -188,7 +188,11 @@ is_attribute_p (const char *attr_name, c
> static inline tree
> lookup_attribute (const char *attr_name, tree list)
> {
> - gcc_checking_assert (attr_name[0] != '_');
> + if (CHECKING_P && attr_name[0] != '_')
> + {
> + size_t attr_len = strlen (attr_name);
> + gcc_checking_assert (!canonicalize_attr_name (attr_name, attr_len));
> + }
> /* In most cases, list is NULL_TREE. */
> if (list == NULL_TREE)
> return NULL_TREE;
> @@ -219,7 +223,8 @@ lookup_attribute_by_prefix (const char *
> size_t attr_len = strlen (attr_name);
> while (list)
> {
> - size_t ident_len = IDENTIFIER_LENGTH (get_attribute_name (list));
> + tree name = get_attribute_name (list);
> + size_t ident_len = IDENTIFIER_LENGTH (name);
>
> if (attr_len > ident_len)
> {
> @@ -227,7 +232,7 @@ lookup_attribute_by_prefix (const char *
> continue;
> }
>
> - const char *p = IDENTIFIER_POINTER (get_attribute_name (list));
> + const char *p = IDENTIFIER_POINTER (name);
> gcc_checking_assert (attr_len == 0 || p[0] != '_');
>
> if (strncmp (attr_name, p, attr_len) == 0)
> --- gcc/attribs.c.jj 2021-11-22 10:06:42.172498397 +0100
> +++ gcc/attribs.c 2021-11-23 14:58:25.076233815 +0100
> @@ -115,12 +115,7 @@ static const struct attribute_spec empty
> static void
> extract_attribute_substring (struct substring *str)
> {
> - if (str->length > 4 && str->str[0] == '_' && str->str[1] == '_'
> - && str->str[str->length - 1] == '_' && str->str[str->length - 2] == '_')
> - {
> - str->length -= 4;
> - str->str += 2;
> - }
> + canonicalize_attr_name (str->str, str->length);
> }
>
> /* Insert an array of attributes ATTRIBUTES into a namespace. This
> @@ -387,7 +382,7 @@ register_scoped_attribute (const struct
>
> /* Attribute names in the table must be in the form 'text' and not
> in the form '__text__'. */
> - gcc_assert (str.length > 0 && str.str[0] != '_');
> + gcc_checking_assert (!canonicalize_attr_name (str.str, str.length));
>
> slot = name_space->attribute_hash
> ->find_slot_with_hash (&str, substring_hash (str.str, str.length),
> --- gcc/testsuite/c-c++-common/Wno-attributes-1.c.jj 2021-11-11 14:35:37.637348034 +0100
> +++ gcc/testsuite/c-c++-common/Wno-attributes-1.c 2021-11-23 15:03:05.426198652 +0100
> @@ -1,6 +1,5 @@
> /* PR c++/101940 */
> -/* { dg-do compile } */
> -/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-do compile { target { c || c++11 } } } */
> /* { dg-additional-options "-Wno-attributes=company::,yoyodyne::attr" } */
> /* { dg-additional-options "-Wno-attributes=c1::attr,c1::attr,c1::__attr__" } */
> /* { dg-additional-options "-Wno-attributes=c2::,c2::attr" } */
> --- gcc/testsuite/c-c++-common/Wno-attributes-2.c.jj 2021-11-11 14:35:37.637348034 +0100
> +++ gcc/testsuite/c-c++-common/Wno-attributes-2.c 2021-11-23 15:03:14.637066079 +0100
> @@ -1,6 +1,5 @@
> /* PR c++/101940 */
> -/* { dg-do compile } */
> -/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-do compile { target { c || c++11 } } } */
>
> #pragma GCC diagnostic ignored_attributes "company::,yoyodyne::attr"
> #pragma GCC diagnostic ignored_attributes "c1::attr,c1::attr,c1::__attr__"
> --- gcc/testsuite/c-c++-common/Wno-attributes-4.c.jj 2021-11-23 15:00:47.584182655 +0100
> +++ gcc/testsuite/c-c++-common/Wno-attributes-4.c 2021-11-23 15:02:23.348804286 +0100
> @@ -0,0 +1,8 @@
> +/* PR middle-end/103365 */
> +/* { dg-do compile { target { c || c++11 } } } */
> +
> +#pragma GCC diagnostic ignored_attributes "foo::_bar"
> +#pragma GCC diagnostic ignored_attributes "_foo::bar"
> +
> +[[foo::_bar]] void foo (void);
> +[[_foo::bar]] void bar (void);
>
> Jakub
>
On Wed, 24 Nov 2021, Jakub Jelinek wrote:
> Hi!
>
> As the patch shows, we have quite a few asserts that we don't call
> lookup_attribute etc. with attr_name that starts with an underscore,
> to make sure nobody is trying to call it with non-canonicalized
> attribute name like "__cold__" instead of "cold".
> We canonicalize only attributes that start with 2 underscores and end
> with 2 underscores though.
> Before Marek's patch, that wasn't an issue, we had no attributes like
> "_foo" or "__bar_" etc., so lookup_scoped_attribute_spec would
> always return NULL for those and we wouldn't try to register them,
> look them up etc., just with -Wattributes would warn about them.
> But now, as the new testcase shows, users can actually request such
> attributes to be ignored, and we ICE for those during
> register_scoped_attribute and when that is fixed, ICE later on when
> somebody uses those attributes because they will be looked up
> to find out that they should be ignored.
>
> So, the following patch instead of or in addition to, depending on
> how performance sensitive a particular spot is, checking that
> attribute doesn't start with underscore allows attribute
> names that start with underscore as long as it doesn't canonicalize
> (i.e. doesn't start and end with 2 underscores).
> In addition to that, I've noticed lookup_attribute_by_prefix
> was calling get_attribute_name twice unnecessarily, and 2 tests
> were running in c++98 mode with -std=c++98 -std=c++11 which IMHO
> isn't useful because -std=c++11 testing is done too when testing
> all language versions.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Richard.
> 2021-11-24 Jakub Jelinek <jakub@redhat.com>
>
> PR middle-end/103365
> * attribs.h (lookup_attribute): Allow attr_name to start with
> underscore, as long as canonicalize_attr_name returns false.
> (lookup_attribute_by_prefix): Don't call get_attribute_name twice.
> * attribs.c (extract_attribute_substring): Reimplement using
> canonicalize_attr_name.
> (register_scoped_attribute): Change gcc_assert into
> gcc_checking_assert, verify !canonicalize_attr_name rather than
> that str.str doesn't start with '_'.
>
> * c-c++-common/Wno-attributes-1.c: Require effective target
> c || c++11 and drop dg-additional-options.
> * c-c++-common/Wno-attributes-2.c: Likewise.
> * c-c++-common/Wno-attributes-4.c: New test.
>
> --- gcc/attribs.h.jj 2021-11-22 10:06:42.173498383 +0100
> +++ gcc/attribs.h 2021-11-23 23:35:13.757972934 +0100
> @@ -188,7 +188,11 @@ is_attribute_p (const char *attr_name, c
> static inline tree
> lookup_attribute (const char *attr_name, tree list)
> {
> - gcc_checking_assert (attr_name[0] != '_');
> + if (CHECKING_P && attr_name[0] != '_')
> + {
> + size_t attr_len = strlen (attr_name);
> + gcc_checking_assert (!canonicalize_attr_name (attr_name, attr_len));
> + }
> /* In most cases, list is NULL_TREE. */
> if (list == NULL_TREE)
> return NULL_TREE;
> @@ -219,7 +223,8 @@ lookup_attribute_by_prefix (const char *
> size_t attr_len = strlen (attr_name);
> while (list)
> {
> - size_t ident_len = IDENTIFIER_LENGTH (get_attribute_name (list));
> + tree name = get_attribute_name (list);
> + size_t ident_len = IDENTIFIER_LENGTH (name);
>
> if (attr_len > ident_len)
> {
> @@ -227,7 +232,7 @@ lookup_attribute_by_prefix (const char *
> continue;
> }
>
> - const char *p = IDENTIFIER_POINTER (get_attribute_name (list));
> + const char *p = IDENTIFIER_POINTER (name);
> gcc_checking_assert (attr_len == 0 || p[0] != '_');
>
> if (strncmp (attr_name, p, attr_len) == 0)
> --- gcc/attribs.c.jj 2021-11-22 10:06:42.172498397 +0100
> +++ gcc/attribs.c 2021-11-23 14:58:25.076233815 +0100
> @@ -115,12 +115,7 @@ static const struct attribute_spec empty
> static void
> extract_attribute_substring (struct substring *str)
> {
> - if (str->length > 4 && str->str[0] == '_' && str->str[1] == '_'
> - && str->str[str->length - 1] == '_' && str->str[str->length - 2] == '_')
> - {
> - str->length -= 4;
> - str->str += 2;
> - }
> + canonicalize_attr_name (str->str, str->length);
> }
>
> /* Insert an array of attributes ATTRIBUTES into a namespace. This
> @@ -387,7 +382,7 @@ register_scoped_attribute (const struct
>
> /* Attribute names in the table must be in the form 'text' and not
> in the form '__text__'. */
> - gcc_assert (str.length > 0 && str.str[0] != '_');
> + gcc_checking_assert (!canonicalize_attr_name (str.str, str.length));
>
> slot = name_space->attribute_hash
> ->find_slot_with_hash (&str, substring_hash (str.str, str.length),
> --- gcc/testsuite/c-c++-common/Wno-attributes-1.c.jj 2021-11-11 14:35:37.637348034 +0100
> +++ gcc/testsuite/c-c++-common/Wno-attributes-1.c 2021-11-23 15:03:05.426198652 +0100
> @@ -1,6 +1,5 @@
> /* PR c++/101940 */
> -/* { dg-do compile } */
> -/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-do compile { target { c || c++11 } } } */
> /* { dg-additional-options "-Wno-attributes=company::,yoyodyne::attr" } */
> /* { dg-additional-options "-Wno-attributes=c1::attr,c1::attr,c1::__attr__" } */
> /* { dg-additional-options "-Wno-attributes=c2::,c2::attr" } */
> --- gcc/testsuite/c-c++-common/Wno-attributes-2.c.jj 2021-11-11 14:35:37.637348034 +0100
> +++ gcc/testsuite/c-c++-common/Wno-attributes-2.c 2021-11-23 15:03:14.637066079 +0100
> @@ -1,6 +1,5 @@
> /* PR c++/101940 */
> -/* { dg-do compile } */
> -/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-do compile { target { c || c++11 } } } */
>
> #pragma GCC diagnostic ignored_attributes "company::,yoyodyne::attr"
> #pragma GCC diagnostic ignored_attributes "c1::attr,c1::attr,c1::__attr__"
> --- gcc/testsuite/c-c++-common/Wno-attributes-4.c.jj 2021-11-23 15:00:47.584182655 +0100
> +++ gcc/testsuite/c-c++-common/Wno-attributes-4.c 2021-11-23 15:02:23.348804286 +0100
> @@ -0,0 +1,8 @@
> +/* PR middle-end/103365 */
> +/* { dg-do compile { target { c || c++11 } } } */
> +
> +#pragma GCC diagnostic ignored_attributes "foo::_bar"
> +#pragma GCC diagnostic ignored_attributes "_foo::bar"
> +
> +[[foo::_bar]] void foo (void);
> +[[_foo::bar]] void bar (void);
>
> Jakub
>
>
On Wed, Nov 24, 2021 at 12:53:02AM -0800, Andrew Pinski wrote:
> Only one comment on the new testcases, you might want to add a
> testcase for the option on the command line too.
You're right, I've committed the patch with the following
incremental diff in it:
--- gcc/testsuite/c-c++-common/Wno-attributes-4.c 2021-11-23 15:02:23.348804286 +0100
+++ gcc/testsuite/c-c++-common/Wno-attributes-4.c 2021-11-24 10:05:20.769192421 +0100
@@ -1,8 +1,7 @@
/* PR middle-end/103365 */
/* { dg-do compile { target { c || c++11 } } } */
-
-#pragma GCC diagnostic ignored_attributes "foo::_bar"
-#pragma GCC diagnostic ignored_attributes "_foo::bar"
+/* { dg-additional-options "-Wno-attributes=foo::_bar" } */
+/* { dg-additional-options "-Wno-attributes=_foo::bar" } */
[[foo::_bar]] void foo (void);
[[_foo::bar]] void bar (void);
--- gcc/testsuite/c-c++-common/Wno-attributes-5.c.jj 2021-11-24 10:04:37.228813482 +0100
+++ gcc/testsuite/c-c++-common/Wno-attributes-5.c 2021-11-24 10:04:20.254055617 +0100
@@ -0,0 +1,8 @@
+/* PR middle-end/103365 */
+/* { dg-do compile { target { c || c++11 } } } */
+
+#pragma GCC diagnostic ignored_attributes "foo::_bar"
+#pragma GCC diagnostic ignored_attributes "_foo::bar"
+
+[[foo::_bar]] void foo (void);
+[[_foo::bar]] void bar (void);
after testing those tests again with vanilla and patched
compiler.
Jakub
@@ -188,7 +188,11 @@ is_attribute_p (const char *attr_name, c
static inline tree
lookup_attribute (const char *attr_name, tree list)
{
- gcc_checking_assert (attr_name[0] != '_');
+ if (CHECKING_P && attr_name[0] != '_')
+ {
+ size_t attr_len = strlen (attr_name);
+ gcc_checking_assert (!canonicalize_attr_name (attr_name, attr_len));
+ }
/* In most cases, list is NULL_TREE. */
if (list == NULL_TREE)
return NULL_TREE;
@@ -219,7 +223,8 @@ lookup_attribute_by_prefix (const char *
size_t attr_len = strlen (attr_name);
while (list)
{
- size_t ident_len = IDENTIFIER_LENGTH (get_attribute_name (list));
+ tree name = get_attribute_name (list);
+ size_t ident_len = IDENTIFIER_LENGTH (name);
if (attr_len > ident_len)
{
@@ -227,7 +232,7 @@ lookup_attribute_by_prefix (const char *
continue;
}
- const char *p = IDENTIFIER_POINTER (get_attribute_name (list));
+ const char *p = IDENTIFIER_POINTER (name);
gcc_checking_assert (attr_len == 0 || p[0] != '_');
if (strncmp (attr_name, p, attr_len) == 0)
@@ -115,12 +115,7 @@ static const struct attribute_spec empty
static void
extract_attribute_substring (struct substring *str)
{
- if (str->length > 4 && str->str[0] == '_' && str->str[1] == '_'
- && str->str[str->length - 1] == '_' && str->str[str->length - 2] == '_')
- {
- str->length -= 4;
- str->str += 2;
- }
+ canonicalize_attr_name (str->str, str->length);
}
/* Insert an array of attributes ATTRIBUTES into a namespace. This
@@ -387,7 +382,7 @@ register_scoped_attribute (const struct
/* Attribute names in the table must be in the form 'text' and not
in the form '__text__'. */
- gcc_assert (str.length > 0 && str.str[0] != '_');
+ gcc_checking_assert (!canonicalize_attr_name (str.str, str.length));
slot = name_space->attribute_hash
->find_slot_with_hash (&str, substring_hash (str.str, str.length),
@@ -1,6 +1,5 @@
/* PR c++/101940 */
-/* { dg-do compile } */
-/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-do compile { target { c || c++11 } } } */
/* { dg-additional-options "-Wno-attributes=company::,yoyodyne::attr" } */
/* { dg-additional-options "-Wno-attributes=c1::attr,c1::attr,c1::__attr__" } */
/* { dg-additional-options "-Wno-attributes=c2::,c2::attr" } */
@@ -1,6 +1,5 @@
/* PR c++/101940 */
-/* { dg-do compile } */
-/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-do compile { target { c || c++11 } } } */
#pragma GCC diagnostic ignored_attributes "company::,yoyodyne::attr"
#pragma GCC diagnostic ignored_attributes "c1::attr,c1::attr,c1::__attr__"
@@ -0,0 +1,8 @@
+/* PR middle-end/103365 */
+/* { dg-do compile { target { c || c++11 } } } */
+
+#pragma GCC diagnostic ignored_attributes "foo::_bar"
+#pragma GCC diagnostic ignored_attributes "_foo::bar"
+
+[[foo::_bar]] void foo (void);
+[[_foo::bar]] void bar (void);