Re: [PATCH v6] attribs: Implement -Wno-attributes=vendor::attr [PR101940]

[Marek Polacek <polacek@redhat.com>]

On Tue, Nov 09, 2021 at 12:12:10AM -0500, Jason Merrill wrote:
> On 11/8/21 20:41, Marek Polacek wrote:
> > On Sat, Nov 06, 2021 at 03:29:57PM -0400, Jason Merrill wrote:
> > > On 11/6/21 14:28, Marek Polacek wrote:
> > > > On Sat, Nov 06, 2021 at 02:32:26AM +0100, Bernhard Reutner-Fischer wrote:
> > > > > On 6 November 2021 01:21:43 CET, Marek Polacek via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> > > > > 
> > > > > > 
> > > > > > Thanks, so like this?  I'm including an incremental diff so that it's
> > > > > > clear what changed:
> > > > > > 
> > > > > > diff --git a/gcc/attribs.c b/gcc/attribs.c
> > > > > > index d5fba7f4bbb..addfe6f6c80 100644
> > > > > > --- a/gcc/attribs.c
> > > > > > +++ b/gcc/attribs.c
> > > > > > @@ -237,7 +237,7 @@ check_attribute_tables (void)
> > > > > >      the end of parsing of all TUs. */
> > > > > > static vec<attribute_spec *> ignored_attributes_table;
> > > > > > 
> > > > > > -/* Parse arguments ARGS of -Wno-attributes=.
> > > > > > +/* Parse arguments V of -Wno-attributes=.
> > > > > >      Currently we accept:
> > > > > >        vendor::attr
> > > > > >        vendor::
> > > > > > @@ -252,12 +252,15 @@ handle_ignored_attributes_option (vec<char *> *v)
> > > > > > 
> > > > > >     for (auto opt : v)
> > > > > >       {
> > > > > > +      /* We're going to be modifying the string.  */
> > > > > > +      opt = xstrdup (opt);
> > > > > >         char *q = strstr (opt, "::");
> > > > > >         /* We don't accept '::attr'.  */
> > > > > >         if (q == nullptr || q == opt)
> > > > > > 	{
> > > > > > 	  error ("wrong argument to ignored attributes");
> > > > > > 	  inform (input_location, "valid format is %<ns::attr%> or %<ns::%>");
> > > > > > +	  free (opt);
> > > > > > 	  continue;
> > > > > > 	}
> > > > > 
> > > > > Only xstrdup here, after the strstr check?
> > > > > Should maybe strdup the rest here, not full opt..
> > > > 
> > > > No, I want q to point into the copy of the string, since I'm about
> > > > to modify it.  And I'd prefer a single call to xstrdup rather than
> > > > two.
> > > 
> > > It occurs to me that instead of calling xstrdup at all, since you're already
> > > passing the strings to get_identifier you could use
> > > get_identifier_with_length instead, and then refer to IDENTIFIER_POINTER of
> > > the result.
> > 
> > Ah, clever.  I didn't think it would work because I didn't expect that
> > get_identifier_with_length works when it gets a string that isn't
> > nul-terminated but it does.  So how about the following?
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > -- >8 --
> > It is desirable for -Wattributes to warn about e.g.
> > 
> > [[deprecate]] void g(); // typo, should warn
> > 
> > However, -Wattributes also warns about vendor-specific attributes
> > (that's because lookup_scoped_attribute_spec -> find_attribute_namespace
> > finds nothing), which, with -Werror, causes grief.  We don't want the
> > -Wattributes warning for
> > 
> > [[company::attr]] void f();
> > 
> > GCC warns because it doesn't know the "company" namespace; it only knows
> > the "gnu" and "omp" namespaces.  We could entirely disable warning about
> > attributes in unknown scopes but then the compiler would also miss typos
> > like
> > 
> >    [[company::attrx]] void f();
> > 
> > or
> > 
> >    [[gmu::warn_used_result]] int write();
> > 
> > so that is not a viable solution.  A workaround is to use a #pragma:
> > 
> >    #pragma GCC diagnostic push
> >    #pragma GCC diagnostic ignored "-Wattributes"
> >    [[company::attr]] void f() {}
> >    #pragma GCC diagnostic pop
> > 
> > but that's a mouthful and awkward to use and could also hide typos.  In
> > fact, any macro-based solution doesn't seem like a way forward.
> > 
> > This patch implements -Wno-attributes=, which takes these arguments:
> > 
> > company::attr
> > company::
> > 
> > This option should go well with using @file: the user could have a file
> > containing
> > -Wno-attributes=vendor::attr1,vendor::attr2
> > and then invoke gcc with '@attrs' or similar.
> > 
> > I've also added a new pragma which has the same effect:
> > 
> > The pragma along with the new option should help with various static
> > analysis tools.
> > 
> > 	PR c++/101940
> > 
> > gcc/ChangeLog:
> > 
> > 	* attribs.c (struct scoped_attributes): Add a bool member.
> > 	(lookup_scoped_attribute_spec): Forward declare.
> > 	(register_scoped_attributes): New bool parameter, defaulted to
> > 	false.  Use it.
> > 	(handle_ignored_attributes_option): New function.
> > 	(free_attr_data): New function.
> > 	(init_attributes): Call handle_ignored_attributes_option.
> > 	(attr_namespace_ignored_p): New function.
> > 	(decl_attributes): Check attr_namespace_ignored_p before
> > 	warning.
> > 	* attribs.h (free_attr_data): Declare.
> > 	(register_scoped_attributes): Adjust declaration.
> > 	(handle_ignored_attributes_option): Declare.
> > 	* common.opt (Wattributes=): New option with a variable.
> > 	* doc/extend.texi: Document #pragma GCC diagnostic ignored_attributes.
> > 	* doc/invoke.texi: Document -Wno-attributes=.
> > 	* opts.c (common_handle_option) <case OPT_Wattributes_>: Handle.
> > 	* plugin.h (register_scoped_attributes): Adjust declaration.
> > 	* toplev.c (compile_file): Call free_attr_data.
> > 
> > gcc/c-family/ChangeLog:
> > 
> > 	* c-pragma.c (handle_pragma_diagnostic): Handle #pragma GCC diagnostic
> > 	ignored_attributes.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* c-c++-common/Wno-attributes-1.c: New test.
> > 	* c-c++-common/Wno-attributes-2.c: New test.
> > ---
> >   gcc/attribs.c                                 | 123 +++++++++++++++++-
> >   gcc/attribs.h                                 |   5 +-
> >   gcc/c-family/c-pragma.c                       |  37 +++++-
> >   gcc/common.opt                                |   9 +-
> >   gcc/doc/extend.texi                           |  19 +++
> >   gcc/doc/invoke.texi                           |  20 +++
> >   gcc/opts.c                                    |  20 +++
> >   gcc/plugin.h                                  |   4 +-
> >   gcc/testsuite/c-c++-common/Wno-attributes-1.c |  55 ++++++++
> >   gcc/testsuite/c-c++-common/Wno-attributes-2.c |  56 ++++++++
> >   gcc/toplev.c                                  |   2 +
> >   11 files changed, 341 insertions(+), 9 deletions(-)
> >   create mode 100644 gcc/testsuite/c-c++-common/Wno-attributes-1.c
> >   create mode 100644 gcc/testsuite/c-c++-common/Wno-attributes-2.c
> > 
> > diff --git a/gcc/attribs.c b/gcc/attribs.c
> > index 83fafc98b7d..23d92ca9474 100644
> > --- a/gcc/attribs.c
> > +++ b/gcc/attribs.c
> > @@ -87,6 +87,8 @@ struct scoped_attributes
> >     const char *ns;
> >     vec<attribute_spec> attributes;
> >     hash_table<attribute_hasher> *attribute_hash;
> > +  /* True if we should not warn about unknown attributes in this NS.  */
> > +  bool ignored_p;
> >   };
> >   /* The table of scope attributes.  */
> > @@ -95,6 +97,8 @@ static vec<scoped_attributes> attributes_table;
> >   static scoped_attributes* find_attribute_namespace (const char*);
> >   static void register_scoped_attribute (const struct attribute_spec *,
> >   				       scoped_attributes *);
> > +static const struct attribute_spec *lookup_scoped_attribute_spec (const_tree,
> > +								  const_tree);
> >   static bool attributes_initialized = false;
> > @@ -121,12 +125,14 @@ extract_attribute_substring (struct substring *str)
> >   /* Insert an array of attributes ATTRIBUTES into a namespace.  This
> >      array must be NULL terminated.  NS is the name of attribute
> > -   namespace.  The function returns the namespace into which the
> > -   attributes have been registered.  */
> > +   namespace.  IGNORED_P is true iff all unknown attributes in this
> > +   namespace should be ignored for the purposes of -Wattributes.  The
> > +   function returns the namespace into which the attributes have been
> > +   registered.  */
> >   scoped_attributes *
> >   register_scoped_attributes (const struct attribute_spec *attributes,
> > -			    const char *ns)
> > +			    const char *ns, bool ignored_p /*=false*/)
> >   {
> >     scoped_attributes *result = NULL;
> > @@ -144,9 +150,12 @@ register_scoped_attributes (const struct attribute_spec *attributes,
> >         memset (&sa, 0, sizeof (sa));
> >         sa.ns = ns;
> >         sa.attributes.create (64);
> > +      sa.ignored_p = ignored_p;
> >         result = attributes_table.safe_push (sa);
> >         result->attribute_hash = new hash_table<attribute_hasher> (200);
> >       }
> > +  else
> > +    result->ignored_p |= ignored_p;
> >     /* Really add the attributes to their namespace now.  */
> >     for (unsigned i = 0; attributes[i].name != NULL; ++i)
> > @@ -224,6 +233,95 @@ check_attribute_tables (void)
> >   				 attribute_tables[j][l].name));
> >   }
> > +/* Used to stash pointers to allocated memory so that we can free them at
> > +   the end of parsing of all TUs. */
> > +static vec<attribute_spec *> ignored_attributes_table;
> > +
> > +/* Parse arguments V of -Wno-attributes=.
> > +   Currently we accept:
> > +     vendor::attr
> > +     vendor::
> > +   This functions also registers the parsed attributes so that we don't
> > +   warn that we don't recognize them.  */
> > +
> > +void
> > +handle_ignored_attributes_option (vec<char *> *v)
> > +{
> > +  if (v == nullptr)
> > +    return;
> > +
> > +  for (auto opt : v)
> > +    {
> > +      char *cln = strstr (opt, "::");
> > +      /* We don't accept '::attr'.  */
> > +      if (cln == nullptr || cln == opt)
> > +	{
> > +	  error ("wrong argument to ignored attributes");
> > +	  inform (input_location, "valid format is %<ns::attr%> or %<ns::%>");
> > +	  continue;
> > +	}
> > +      char *vendor_start = opt;
> > +      ptrdiff_t vendor_len = cln - opt;
> > +      char *attr_start = cln + 2;
> > +      /* This could really use rawmemchr :(.  */
> > +      ptrdiff_t attr_len = strchr (attr_start, '\0') - attr_start;
> > +      /* Verify that they look valid.  */
> > +      auto valid_p = [](const char *const s, ptrdiff_t len) {
> > +	for (int i = 0; i < len; ++i)
> > +	  if (!ISALNUM (*s) && *s != '_')
> > +	    return false;
> > +	return true;
> > +      };
> > +      if (!valid_p (vendor_start, vendor_len)
> > +	  || !valid_p (attr_start, attr_len))
> > +	{
> > +	  error ("wrong argument to ignored attributes");
> > +	  continue;
> > +	}
> > +      /* Turn "__attr__" into "attr" so that we have a canonical form of
> > +	 attribute names.  Likewise for vendor.  */
> > +      auto strip = [](char *&s, ptrdiff_t &l) {
> > +	if (l > 4 && s[0] == '_' && s[1] == '_'
> > +	    && s[l - 1] == '_' && s[l - 2] == '_')
> > +	  {
> > +	    s += 2;
> > +	    l -= 4;
> > +	  }
> > +      };
> > +      strip (attr_start, attr_len);
> > +      strip (vendor_start, vendor_len);
> > +      /* We perform all this hijinks so that we don't have to copy OPT.  */
> > +      tree vendor_id = get_identifier_with_length (vendor_start, vendor_len);
> > +      tree attr_id = get_identifier_with_length (attr_start, attr_len);
> > +      /* If we've already seen this vendor::attr, ignore it.  Attempting to
> > +	 register it twice would lead to a crash.  */
> > +      if (lookup_scoped_attribute_spec (vendor_id, attr_id))
> > +	continue;
> 
> Hmm, this looks like it isn't handling the case of previously ignoring
> vendor::; it seems we'll look for vendor::<empty string> instead, not find
> it, and register again.

Yes, for -Wno-attributes=vendor::,vendor:: we call register_scoped_attributes
twice, but I think that's OK: register_scoped_attributes will see that 
find_attribute_namespace finds the namespace and returns without creating a new  
one.

I think the current code handles -Wno-attributes=vendor::a,vendor:: well so
I'm not sure if I should change it.

Thanks,
Marek