attribs: Fix ICE with -Wno-attributes= [PR112339]

  Hi!

The following testcase ICEs, because with -Wno-attributes=foo::no_sanitize
(but generally any other non-gnu namespace and some gnu well known attribute
name within that other namespace) the FEs don't really parse attribute
arguments of such attribute, but lookup_attribute_spec is non-NULL with
NULL handler and such attributes are added to DECL_ATTRIBUTES or
TYPE_ATTRIBUTES and then when e.g. middle-end does lookup_attribute
on a particular attribute and expects the attribute to mean something
and/or have a particular verified arguments, it can crash when seeing
the foreign attribute in there instead.

The following patch fixes that by never adding ignored attributes
to DECL_ATTRIBUTES/TYPE_ATTRIBUTES, previously that was the case just
for attributes in ignored namespace (where lookup_attribute_space
returned NULL).  We don't really know anything about those attributes,
so shouldn't pretend we know something about them, especially when
the arguments are error_mark_node or NULL instead of something that
would have been parsed.  And it would be really weird if we normally
ignore say [[clang::unused]] attribute, but when people use
-Wno-attributes=clang::unused we actually treated it as gnu::unused.
All the user asked for is suppress warnings about that attribute being
unknown.

The first hunk is just playing safe, I'm worried people could
-Wno-attributes=gnu::
and get various crashes with known GNU attributes not being actually
parsed and recorded (or worse e.g. when we tweak standard attributes
into GNU attributes and we wouldn't add those).
The -Wno-attributes= documentation says that it suppresses warning about
unknown attributes, so I think -Wno-attributes=gnu:: should prevent
warning about say [[gnu::foobarbaz]] attribute, but not about
[[gnu::unused]] because the latter is a known attribute.
The routine would return true for any scoped attribute in the ignored
namespace, with the change it ignores only unknown attributes in ignored
namespace, known ones in there will be ignored only if they have
max_length of -2 (e.g.. with
-Wno-attributes=gnu:: -Wno-attributes=gnu::foobarbaz).

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2023-11-03  Jakub Jelinek  <jakub@redhat.com>

	PR c/112339
	* attribs.cc (attribute_ignored_p): Only return true for
	attr_namespace_ignored_p if as is NULL.
	(decl_attributes): Never add ignored attributes.

	* c-c++-common/ubsan/Wno-attributes-1.c: New test.


	Jakub
  

LGTM but I'd like Marek to approve it.

On Fri, Nov 3, 2023, 3:12 PM Jakub Jelinek <jakub@redhat.com> wrote:

> Hi!
>
> The following testcase ICEs, because with -Wno-attributes=foo::no_sanitize
> (but generally any other non-gnu namespace and some gnu well known
> attribute
> name within that other namespace) the FEs don't really parse attribute
> arguments of such attribute, but lookup_attribute_spec is non-NULL with
> NULL handler and such attributes are added to DECL_ATTRIBUTES or
> TYPE_ATTRIBUTES and then when e.g. middle-end does lookup_attribute
> on a particular attribute and expects the attribute to mean something
> and/or have a particular verified arguments, it can crash when seeing
> the foreign attribute in there instead.
>
> The following patch fixes that by never adding ignored attributes
> to DECL_ATTRIBUTES/TYPE_ATTRIBUTES, previously that was the case just
> for attributes in ignored namespace (where lookup_attribute_space
> returned NULL).  We don't really know anything about those attributes,
> so shouldn't pretend we know something about them, especially when
> the arguments are error_mark_node or NULL instead of something that
> would have been parsed.  And it would be really weird if we normally
> ignore say [[clang::unused]] attribute, but when people use
> -Wno-attributes=clang::unused we actually treated it as gnu::unused.
> All the user asked for is suppress warnings about that attribute being
> unknown.
>
> The first hunk is just playing safe, I'm worried people could
> -Wno-attributes=gnu::
> and get various crashes with known GNU attributes not being actually
> parsed and recorded (or worse e.g. when we tweak standard attributes
> into GNU attributes and we wouldn't add those).
> The -Wno-attributes= documentation says that it suppresses warning about
> unknown attributes, so I think -Wno-attributes=gnu:: should prevent
> warning about say [[gnu::foobarbaz]] attribute, but not about
> [[gnu::unused]] because the latter is a known attribute.
> The routine would return true for any scoped attribute in the ignored
> namespace, with the change it ignores only unknown attributes in ignored
> namespace, known ones in there will be ignored only if they have
> max_length of -2 (e.g.. with
> -Wno-attributes=gnu:: -Wno-attributes=gnu::foobarbaz).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2023-11-03  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c/112339
>         * attribs.cc (attribute_ignored_p): Only return true for
>         attr_namespace_ignored_p if as is NULL.
>         (decl_attributes): Never add ignored attributes.
>
>         * c-c++-common/ubsan/Wno-attributes-1.c: New test.
>
> --- gcc/attribs.cc.jj   2023-11-02 20:22:02.017016371 +0100
> +++ gcc/attribs.cc      2023-11-03 08:45:32.688726738 +0100
> @@ -579,9 +579,9 @@ attribute_ignored_p (tree attr)
>      return false;
>    if (tree ns = get_attribute_namespace (attr))
>      {
> -      if (attr_namespace_ignored_p (ns))
> -       return true;
>        const attribute_spec *as = lookup_attribute_spec (TREE_PURPOSE
> (attr));
> +      if (as == NULL && attr_namespace_ignored_p (ns))
> +       return true;
>        if (as && as->max_length == -2)
>         return true;
>      }
> @@ -862,7 +862,10 @@ decl_attributes (tree *node, tree attrib
>             }
>         }
>
> -      if (no_add_attrs)
> +      if (no_add_attrs
> +         /* Don't add attributes registered just for
> -Wno-attributes=foo::bar
> +            purposes.  */
> +         || attribute_ignored_p (attr))
>         continue;
>
>        if (spec->handler != NULL)
> --- gcc/testsuite/c-c++-common/ubsan/Wno-attributes-1.c.jj      2023-11-03
> 08:44:13.752837201 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/Wno-attributes-1.c 2023-11-03
> 08:44:13.751837215 +0100
> @@ -0,0 +1,9 @@
> +/* PR c/112339 */
> +/* { dg-do compile { target { c++11 || c } } } */
> +/* { dg-options "-Wno-attributes=foo::no_sanitize -fsanitize=undefined" }
> */
> +/* { dg-additional-options "-std=c2x" { target c } } */
> +
> +[[foo::no_sanitize("bounds")]] void
> +foo (void)
> +{
> +}
>
>         Jakub
>
>
  

On Fri, Nov 03, 2023 at 06:43:49PM -0400, Jason Merrill wrote:
> LGTM but I'd like Marek to approve it.

Both hunks look correct to me.  Patch is OK, thanks!
 
> On Fri, Nov 3, 2023, 3:12 PM Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > Hi!
> >
> > The following testcase ICEs, because with -Wno-attributes=foo::no_sanitize
> > (but generally any other non-gnu namespace and some gnu well known
> > attribute
> > name within that other namespace) the FEs don't really parse attribute
> > arguments of such attribute, but lookup_attribute_spec is non-NULL with
> > NULL handler and such attributes are added to DECL_ATTRIBUTES or
> > TYPE_ATTRIBUTES and then when e.g. middle-end does lookup_attribute
> > on a particular attribute and expects the attribute to mean something
> > and/or have a particular verified arguments, it can crash when seeing
> > the foreign attribute in there instead.
> >
> > The following patch fixes that by never adding ignored attributes
> > to DECL_ATTRIBUTES/TYPE_ATTRIBUTES, previously that was the case just
> > for attributes in ignored namespace (where lookup_attribute_space
> > returned NULL).  We don't really know anything about those attributes,
> > so shouldn't pretend we know something about them, especially when
> > the arguments are error_mark_node or NULL instead of something that
> > would have been parsed.  And it would be really weird if we normally
> > ignore say [[clang::unused]] attribute, but when people use
> > -Wno-attributes=clang::unused we actually treated it as gnu::unused.
> > All the user asked for is suppress warnings about that attribute being
> > unknown.
> >
> > The first hunk is just playing safe, I'm worried people could
> > -Wno-attributes=gnu::
> > and get various crashes with known GNU attributes not being actually
> > parsed and recorded (or worse e.g. when we tweak standard attributes
> > into GNU attributes and we wouldn't add those).
> > The -Wno-attributes= documentation says that it suppresses warning about
> > unknown attributes, so I think -Wno-attributes=gnu:: should prevent
> > warning about say [[gnu::foobarbaz]] attribute, but not about
> > [[gnu::unused]] because the latter is a known attribute.
> > The routine would return true for any scoped attribute in the ignored
> > namespace, with the change it ignores only unknown attributes in ignored
> > namespace, known ones in there will be ignored only if they have
> > max_length of -2 (e.g.. with
> > -Wno-attributes=gnu:: -Wno-attributes=gnu::foobarbaz).
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > 2023-11-03  Jakub Jelinek  <jakub@redhat.com>
> >
> >         PR c/112339
> >         * attribs.cc (attribute_ignored_p): Only return true for
> >         attr_namespace_ignored_p if as is NULL.
> >         (decl_attributes): Never add ignored attributes.
> >
> >         * c-c++-common/ubsan/Wno-attributes-1.c: New test.
> >
> > --- gcc/attribs.cc.jj   2023-11-02 20:22:02.017016371 +0100
> > +++ gcc/attribs.cc      2023-11-03 08:45:32.688726738 +0100
> > @@ -579,9 +579,9 @@ attribute_ignored_p (tree attr)
> >      return false;
> >    if (tree ns = get_attribute_namespace (attr))
> >      {
> > -      if (attr_namespace_ignored_p (ns))
> > -       return true;
> >        const attribute_spec *as = lookup_attribute_spec (TREE_PURPOSE
> > (attr));
> > +      if (as == NULL && attr_namespace_ignored_p (ns))
> > +       return true;
> >        if (as && as->max_length == -2)
> >         return true;
> >      }
> > @@ -862,7 +862,10 @@ decl_attributes (tree *node, tree attrib
> >             }
> >         }
> >
> > -      if (no_add_attrs)
> > +      if (no_add_attrs
> > +         /* Don't add attributes registered just for
> > -Wno-attributes=foo::bar
> > +            purposes.  */
> > +         || attribute_ignored_p (attr))
> >         continue;
> >
> >        if (spec->handler != NULL)
> > --- gcc/testsuite/c-c++-common/ubsan/Wno-attributes-1.c.jj      2023-11-03
> > 08:44:13.752837201 +0100
> > +++ gcc/testsuite/c-c++-common/ubsan/Wno-attributes-1.c 2023-11-03
> > 08:44:13.751837215 +0100
> > @@ -0,0 +1,9 @@
> > +/* PR c/112339 */
> > +/* { dg-do compile { target { c++11 || c } } } */
> > +/* { dg-options "-Wno-attributes=foo::no_sanitize -fsanitize=undefined" }
> > */
> > +/* { dg-additional-options "-std=c2x" { target c } } */
> > +
> > +[[foo::no_sanitize("bounds")]] void
> > +foo (void)
> > +{
> > +}
> >
> >         Jakub
> >
> >

Marek