LGTM but I'd like Marek to approve it.
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
>
>
