From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id C80CC3858401 for ; Tue, 9 Nov 2021 15:51:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C80CC3858401 Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-90-iqbZAxs5MfecyhNZp-kXBA-1; Tue, 09 Nov 2021 10:51:56 -0500 X-MC-Unique: iqbZAxs5MfecyhNZp-kXBA-1 Received: by mail-qv1-f70.google.com with SMTP id jn10-20020ad45dea000000b003bd74c93df4so17808963qvb.15 for ; Tue, 09 Nov 2021 07:51:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=tnVhQfvInjhUYYwS21LkpjIECurGFjCIZTWobKNATj0=; b=Y1jn/SzTH5tjIDo/NV2CDWyHxCJXKqd4ImGDZPL76mhgXCHYRmCHCInwUy3g7RGU9p VaFRYhdhsx4X6Vh5z2i7GT+4i633NRthIqwq5hnvW0jGbrg9ZcxbSIPn1UeafBcH1H5K fWKKMuTEsV9vfq056KCxvha2G9Yu7mVwyIJnQ+j8XuzgVX1kafPAxqBpG8EI7ZTC/aXx NU2ncHkYeyjuivj/TUOJtvqchaGrcI1DBAxODW/ZLE0Dst17i0vtzeZtM1U8bdRbhqTW ksjEVjgt6Ch2ekR5T2W4U3RH8UAKv7eyjTAUBsKZUnuiGxT6MbZhSItFEhLd7g2Im116 RJVw== X-Gm-Message-State: AOAM533LQ52lOe/7kll0PKk7nRgHREsGRFQVW55bFKeQ0k/2Os1CVBGU PqWzEx+DtZWL8Pn8HjVjbwIgDOOShPLVHwgIY0STAZdPBrTUhvgAQwjMWoz24LlmzlD/QDCbLuw 7WAvqu7EkgTMaaWWxrQ== X-Received: by 2002:a0c:eb49:: with SMTP id c9mr8350687qvq.30.1636473116383; Tue, 09 Nov 2021 07:51:56 -0800 (PST) X-Google-Smtp-Source: ABdhPJxs8WPsfQh12sdjEPy3XZcnBhpc27A+yldww9gjctXcs9CINMEhM7vNFu5WOwYQGk7Z8/aw5Q== X-Received: by 2002:a0c:eb49:: with SMTP id c9mr8350646qvq.30.1636473116092; Tue, 09 Nov 2021 07:51:56 -0800 (PST) Received: from redhat.com ([2601:184:4780:4310::aac2]) by smtp.gmail.com with ESMTPSA id bm27sm1772775qkb.4.2021.11.09.07.51.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 09 Nov 2021 07:51:55 -0800 (PST) Date: Tue, 9 Nov 2021 10:51:53 -0500 From: Marek Polacek To: Jason Merrill Cc: Bernhard Reutner-Fischer , Marek Polacek via Gcc-patches , Jakub Jelinek Subject: Re: [PATCH v6] attribs: Implement -Wno-attributes=vendor::attr [PR101940] Message-ID: References: <8636350f-e588-12d1-b687-89245de0b62d@redhat.com> <89690210-3ac3-9486-1fa0-742fc67d3748@redhat.com> <84D9B2CE-EA9B-46FB-9BF5-809D29B7018E@gmail.com> <516f3ddd-8634-9d73-ddd4-ae470f4284cf@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.1.3 (2021-09-10) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Nov 2021 15:52:01 -0000 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 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 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 *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 % or %"); > > > > > > + 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) : 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 attributes; > > hash_table *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 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 (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 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 *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 % or %"); > > + 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:: 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