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 <ja...@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 <ja...@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
