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
