Message ID | 20211124084746.GJ2646553@tucnak |
---|---|
State | New |
Headers | show |
Series | attribs: Fix ICEs on attributes starting with _ [PR103365] | expand |
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
--- 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);