Message ID | 20220323130117.525495-1-dmalcolm@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 59BC73858427 for <patchwork@sourceware.org>; Wed, 23 Mar 2022 13:03:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 59BC73858427 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1648040617; bh=PMpXcmNmRPqeEEhZ/Ab63UMb1iJIqwwbkuXnezJzUw0=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=iFwus9sOSCfmVyQb66NhOhW1EN7jAdZAJDxBZAdNX+Blg8PQ/g0h1EOOx/C46TKkQ LNH/XaCQmgOkJ7VnihkICmbzHAv1s7Sr7LL+Jr2FezJmm5eIHLbxMcpwD8SxNGsGSv MzxrFAt3gMX6u0c1Gz2LKd8ryM6uXX0oXyXBEVNg= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 0F4983858002 for <gcc-patches@gcc.gnu.org>; Wed, 23 Mar 2022 13:01:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0F4983858002 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-637-UqZdva9EMj-9RFh5g9uv5Q-1; Wed, 23 Mar 2022 09:01:23 -0400 X-MC-Unique: UqZdva9EMj-9RFh5g9uv5Q-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id DAA3A3C01C30; Wed, 23 Mar 2022 13:01:22 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.17.17]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9C5E640470C0; Wed, 23 Mar 2022 13:01:22 +0000 (UTC) To: gcc-patches@gcc.gnu.org, gcc@gcc.gnu.org Subject: [PATCH v2] Document that the 'access' and 'nonnull' attributes are independent Date: Wed, 23 Mar 2022 09:01:17 -0400 Message-Id: <20220323130117.525495-1-dmalcolm@redhat.com> In-Reply-To: <bfcd0ef2-c2bb-fc9a-5e06-487e85a8db6c@gmail.com> References: <bfcd0ef2-c2bb-fc9a-5e06-487e85a8db6c@gmail.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII" X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=unavailable 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: David Malcolm via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: David Malcolm <dmalcolm@redhat.com> Cc: Martin Sebor <msebor@gmail.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[v2] Document that the 'access' and 'nonnull' attributes are independent
|
|
Commit Message
David Malcolm
March 23, 2022, 1:01 p.m. UTC
On Mon, 2022-03-14 at 16:18 -0600, Martin Sebor wrote: > On 3/9/22 14:57, David Malcolm via Gcc wrote: > > On Wed, 2022-03-09 at 13:30 -0800, Andrew Pinski wrote: > > > On Wed, Mar 9, 2022 at 1:25 PM David Malcolm via Gcc > > > <gcc@gcc.gnu.org> wrote: > > > > > > > > We gained __attribute__ ((access, ...)) in GCC 10: > > > > > > > > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html > > > > which identifies one of the pointer/reference arguments of a > > > > function > > > > as being accessed according to an access-mode: read_only, > > > > read_write, > > > > write_only, or none. > > > > > > > > We also have __attribute__ ((nonnull)) to indicate that a > > > > function > > > > argument (or all of them) must be non-NULL. > > > > > > > > There doesn't seem to be a relationship between these in the > > > > implementation, but it strikes me that almost anywhere that a > > > > user > > > > might use the "access" attribute, that parameter is probably > > > > going > > > > to > > > > be required to be nonnull - though perhaps there are cases > > > > where > > > > APIs > > > > check for NULL and reject them gracefully? > > > > > > No, I think they are separate. The access just says these access > > > attributes are read only, write only, read-write or don't access > > > what > > > the pointer points to; it does not say they have to be read or > > > written > > > to. > > > I think it is a bad idea to connect the two ideas because you > > > could > > > have some cases where an argument is optional but is only read > > > from; > > > or is only written to (there are many in GCC sources even). > > > > Thanks for the clarification... > > > > > > > > Thanks, > > > Andrew Pinski > > > > > > > > > > > Might we want to somehow make __attribute__ ((access, ...)) > > > > imply > > > > __attribute__ ((nonnull))? (for non "none" access modes, > > > > perhaps?) > > > > > > > > If so, one place to implement this might be in tree.cc's > > > > get_nonnull_args, and have it add to the bitmap any arguments > > > > that > > > > have an appropriate access attribute. > > > > > > > > get_nonnull_args is used in various places: > > > > - validating builtins > > > > - in ranger_cache::block_apply_nonnull > > > > - by -Wnonnull (in pass_post_ipa_warn::execute) > > > > - by -Wanalyzer-possible-null-argument and -Wanalyzer-null- > > > > argument; > > > > I'm tracking the failure of these last two to make use of > > > > __attribute__ > > > > ((access)) in PR analyzer/104860. > > > > > > > > So do we: > > > > > > > > (a) leave it up to the user, requiring them to specify > > > > __attribute__ > > > > ((nonnull)) in addition to __attribute__ ((access, ...)) > > > > ...so that's (a) then. > > > > I think it might be more user-friendly to be explicit about this in > > the > > documentation, maybe something like the attached? > > I agree it's worth clarifying the manual. > > But I don't think there's a way to annotate a function to indicate > that it will definitely access an object (or dereference a pointer). > Attribute access just implies that it might dereference it (unless > the size is zero), and attribute nonnull that the pointer must not > be null, not that it will be dereferenced (or even that it must be > valid, although that's implied by the language and should probably > be enforced in all contexts by some other warning). > > The combination of access with nonzero size and nonnull only means > that the pointer must be nonnull and point to an object with at least > size elements. Here's an updated version of the patch which expresses that also. OK for trunk? Dave > > Martin > > > > > (not yet fully tested, but seems to build) > > > > Dave > > > > > > > > > > > > > > > > (b) leave it up to the individual sites in GCC that currently > > > > make > > > > use > > > > of get_nonnull_args to add logic for handling __attribute__ > > > > ((access, > > > > ...)) > > > > > > > > (c) extend get_nonnull_args > > > > > > > > ? > > > > > > > > Thoughts? > > > > Dave > > > > > > > > > > gcc/ChangeLog: * doc/extend.texi (Common Function Attributes): Document that 'access' does not imply 'nonnull'. Signed-off-by: David Malcolm <dmalcolm@redhat.com> --- gcc/doc/extend.texi | 9 +++++++++ 1 file changed, 9 insertions(+)
Comments
On 3/23/22 07:01, David Malcolm wrote: > On Mon, 2022-03-14 at 16:18 -0600, Martin Sebor wrote: >> On 3/9/22 14:57, David Malcolm via Gcc wrote: >>> On Wed, 2022-03-09 at 13:30 -0800, Andrew Pinski wrote: >>>> On Wed, Mar 9, 2022 at 1:25 PM David Malcolm via Gcc >>>> <gcc@gcc.gnu.org> wrote: >>>>> >>>>> We gained __attribute__ ((access, ...)) in GCC 10: >>>>> >>>>> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html >>>>> which identifies one of the pointer/reference arguments of a >>>>> function >>>>> as being accessed according to an access-mode: read_only, >>>>> read_write, >>>>> write_only, or none. >>>>> >>>>> We also have __attribute__ ((nonnull)) to indicate that a >>>>> function >>>>> argument (or all of them) must be non-NULL. >>>>> >>>>> There doesn't seem to be a relationship between these in the >>>>> implementation, but it strikes me that almost anywhere that a >>>>> user >>>>> might use the "access" attribute, that parameter is probably >>>>> going >>>>> to >>>>> be required to be nonnull - though perhaps there are cases >>>>> where >>>>> APIs >>>>> check for NULL and reject them gracefully? >>>> >>>> No, I think they are separate. The access just says these access >>>> attributes are read only, write only, read-write or don't access >>>> what >>>> the pointer points to; it does not say they have to be read or >>>> written >>>> to. >>>> I think it is a bad idea to connect the two ideas because you >>>> could >>>> have some cases where an argument is optional but is only read >>>> from; >>>> or is only written to (there are many in GCC sources even). >>> >>> Thanks for the clarification... >>> >>>> >>>> Thanks, >>>> Andrew Pinski >>>> >>>>> >>>>> Might we want to somehow make __attribute__ ((access, ...)) >>>>> imply >>>>> __attribute__ ((nonnull))? (for non "none" access modes, >>>>> perhaps?) >>>>> >>>>> If so, one place to implement this might be in tree.cc's >>>>> get_nonnull_args, and have it add to the bitmap any arguments >>>>> that >>>>> have an appropriate access attribute. >>>>> >>>>> get_nonnull_args is used in various places: >>>>> - validating builtins >>>>> - in ranger_cache::block_apply_nonnull >>>>> - by -Wnonnull (in pass_post_ipa_warn::execute) >>>>> - by -Wanalyzer-possible-null-argument and -Wanalyzer-null- >>>>> argument; >>>>> I'm tracking the failure of these last two to make use of >>>>> __attribute__ >>>>> ((access)) in PR analyzer/104860. >>>>> >>>>> So do we: >>>>> >>>>> (a) leave it up to the user, requiring them to specify >>>>> __attribute__ >>>>> ((nonnull)) in addition to __attribute__ ((access, ...)) >>> >>> ...so that's (a) then. >>> >>> I think it might be more user-friendly to be explicit about this in >>> the >>> documentation, maybe something like the attached? >> >> I agree it's worth clarifying the manual. >> >> But I don't think there's a way to annotate a function to indicate >> that it will definitely access an object (or dereference a pointer). >> Attribute access just implies that it might dereference it (unless >> the size is zero), and attribute nonnull that the pointer must not >> be null, not that it will be dereferenced (or even that it must be >> valid, although that's implied by the language and should probably >> be enforced in all contexts by some other warning). >> >> The combination of access with nonzero size and nonnull only means >> that the pointer must be nonnull and point to an object with at least >> size elements. > > Here's an updated version of the patch which expresses that also. > > OK for trunk? > > Dave > >> >> Martin >> >>> >>> (not yet fully tested, but seems to build) >>> >>> Dave >>> >>> >>> >>> >>>>> >>>>> (b) leave it up to the individual sites in GCC that currently >>>>> make >>>>> use >>>>> of get_nonnull_args to add logic for handling __attribute__ >>>>> ((access, >>>>> ...)) >>>>> >>>>> (c) extend get_nonnull_args >>>>> >>>>> ? >>>>> >>>>> Thoughts? >>>>> Dave >>>>> >>>> >>> >> > > gcc/ChangeLog: > * doc/extend.texi (Common Function Attributes): Document that > 'access' does not imply 'nonnull'. > > Signed-off-by: David Malcolm <dmalcolm@redhat.com> > --- > gcc/doc/extend.texi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > index a4a25e86928..790014f3da5 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > @@ -2652,6 +2652,15 @@ The mode is intended to be used as a means to help validate the expected > object size, for example in functions that call @code{__builtin_object_size}. > @xref{Object Size Checking}. > > +Note that the @code{access} attribute merely specifies how an object > +referenced by the pointer argument can be accessed; it does not imply that > +an access @strong{will} happen. If the pointer will definitely be > +dereferenced, you may want to also add > +@code{__attribute__((nonnull (@var{arg-index})))} to the function for the > +pointer parameter in question (though the presence of the @code{nonnull} > +attribute does not imply that the parameter will be dereferenced, merely > +that it must be non-null). > + > @item alias ("@var{target}") > @cindex @code{alias} function attribute > The @code{alias} attribute causes the declaration to be emitted as an alias I'm sorry, I probably wasn't clear. This text sounds contradictory: If the pointer will definitely be dereferenced ... does not imply that the parameter will be dereferenced... The concern is that the constraints implied by atttributes access and nonnull are independent of each other. I would suggest to document that without talking about dereferencing because that's not implied by either of them. E.g., something like this (feel free to tweak it as you see fit): Note that the @code{access} attribute doesn't imply the same constraint as attribute @code{nonnull} (@pxref{Attribute nonnull}). The latter attribute should be used to annotate arguments that must never be null, regardless of the value of the size argument. A couple of tests that show the difference are gcc.dg/Wstringop-overflow-{23,24}.c. GCC issues -Wnonnull for just a subset of calls to functions with attribute access with a null pointer (those with a nonzero size argument). Adding attribute nonnull would make it issue -Wnonnull in all of them. Martin
On 23/03/2022 17:31, Martin Sebor via Gcc-patches wrote: > > The concern is that the constraints implied by atttributes access and > nonnull are independent of each other. I would suggest to document > that without talking about dereferencing because that's not implied > by either of them. E.g., something like this (feel free to tweak it > as you see fit): > > Note that the @code{access} attribute doesn't imply the same > constraint as attribute @code{nonnull} (@pxref{Attribute nonnull}). > The latter attribute should be used to annotate arguments that must > never be null, regardless of the value of the size argument. I would not give an advice on using the nonnull attribute here. This attribute could have pretty dangerous effects in the function definition (removal of null pointer checks).
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi index a4a25e86928..790014f3da5 100644 --- a/gcc/doc/extend.texi +++ b/gcc/doc/extend.texi @@ -2652,6 +2652,15 @@ The mode is intended to be used as a means to help validate the expected object size, for example in functions that call @code{__builtin_object_size}. @xref{Object Size Checking}. +Note that the @code{access} attribute merely specifies how an object +referenced by the pointer argument can be accessed; it does not imply that +an access @strong{will} happen. If the pointer will definitely be +dereferenced, you may want to also add +@code{__attribute__((nonnull (@var{arg-index})))} to the function for the +pointer parameter in question (though the presence of the @code{nonnull} +attribute does not imply that the parameter will be dereferenced, merely +that it must be non-null). + @item alias ("@var{target}") @cindex @code{alias} function attribute The @code{alias} attribute causes the declaration to be emitted as an alias