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.129.124]) by sourceware.org (Postfix) with ESMTPS id B34513858C83 for ; Tue, 19 Jul 2022 18:18:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B34513858C83 Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-195-EGBNjlRJMI6mOlcADJZDQA-1; Tue, 19 Jul 2022 14:18:15 -0400 X-MC-Unique: EGBNjlRJMI6mOlcADJZDQA-1 Received: by mail-qt1-f197.google.com with SMTP id r5-20020ac85c85000000b0031ecf611c64so10824362qta.23 for ; Tue, 19 Jul 2022 11:18:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=NUihsobOBGjCYKgviS46penegqGBAzehLJvOXD4SXzc=; b=eysC5mSmimtorC6GfbFZNsbNeGX1k3WE7k0dY8ix0bb0PuC2NRkFVkBJ7JAcoh0o13 Fyy8EeLotovjwl7c0usLHR3rcmlSy3x/lS9GW+J3k3IDjctcKN5qV33kcRXddtwq6QtU kF3DyEGemBMw8u+Np8UmrR4NMk++C4Yl5bMKElGnXuSVDjg0sqeT1EyTLU9bFPnmMPWn lbRiPv1QPfdjvfPjnpftRp8rLPQMdY6SBpwulw/A/zWo6kc4Kog8upqkHxjKseBDJP3c WJZmE4DaqkwodgUWS56ve3FsMw+GG5iDuP0mkroYG9/L/k+eDESLIj9XbyA9aUgf04PK SjtQ== X-Gm-Message-State: AJIora/Afgivv3TOqY9UiVe1YUpiDF4aZQm6JO1RpAqrnmRGnkpWtaVH slVHEsMc6fqUpehQSfEZuNsaKeoc5yOn+pRulsADWzkMlgQ3gKMylTfxk3IzQqMt2PXhYeT4eB+ 8l+fcURyl4gqm1FcRfA== X-Received: by 2002:a05:622a:64b:b0:31d:47f1:1507 with SMTP id a11-20020a05622a064b00b0031d47f11507mr26136150qtb.688.1658254694953; Tue, 19 Jul 2022 11:18:14 -0700 (PDT) X-Google-Smtp-Source: AGRyM1sPh4GjrX3oJaLNps8O3IEC6vIrebPhuHWRJz3srjWUh2d2JjmpDf9b2EDdzKbgPCUJCyhs4A== X-Received: by 2002:a05:622a:64b:b0:31d:47f1:1507 with SMTP id a11-20020a05622a064b00b0031d47f11507mr26136125qtb.688.1658254694589; Tue, 19 Jul 2022 11:18:14 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.ma.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id s11-20020a05620a0bcb00b006b5c5987ff2sm13742637qki.96.2022.07.19.11.18.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Jul 2022 11:18:14 -0700 (PDT) Message-ID: <1e44d1ae1933ed588ee91fbc296683a7428f18e6.camel@redhat.com> Subject: Re: [PATCH] Adding three new function attributes for static analysis of file descriptors From: David Malcolm To: mirimnan017@gmail.com, gcc-patches@gcc.gnu.org Cc: Immad Mir Date: Tue, 19 Jul 2022 14:18:12 -0400 In-Reply-To: References: User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.6 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, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 19 Jul 2022 18:18:22 -0000 On Tue, 2022-07-19 at 21:36 +0530, Immad Mir wrote: [...snip...] Thanks for the patch. It's nearly ready for trunk; I have some review comments below, mostly nits, but a couple of other issues... > gcc/ChangeLog: > * doc/extend.texi: Add fd_arg, fd_arg_read and fd_arg_write under > "Common Function Attributes" section. As well as these additions, please can you also update doc/invoke.texi. Specifically, please update the description of the three warnings that are affected by these attributes so that they refer to the new attributes, rather than just to "read" and "write", so that as well as the docs for the attributes referring to the warnings, the docs for the warnings refer to the attributes. > gcc/testsuite/ChangeLog: > * gcc.dg/analyzer/fd-5.c: New test. > * c-c++-common/attr-fd.c: New test. > > Signed-off-by: Immad Mir [...snip...] > /* Base diagnostic class relative to fd_state_machine. */ > class fd_diagnostic : public pending_diagnostic > { > -public: > +public: There's what looks like an accidental change here, adding a couple of stray spaces after the trailing colon (I think); please fix, to avoid adding noise to the git history. [...snip...] > + void > + inform_filedescriptor_attribute (access_directions fd_dir) > + { > + > + if (m_attr) > + switch (fd_dir) > + { > + case DIRS_READ_WRITE: > + inform (DECL_SOURCE_LOCATION (m_callee_fndecl), > + "argument %d of %qD must be an open file descriptor", > + m_arg_idx + 1, m_callee_fndecl); > + break; > + case DIRS_WRITE: > + inform (DECL_SOURCE_LOCATION (m_callee_fndecl), > + "argument %d of %qD must be a read-only file descriptor", > + m_arg_idx + 1, m_callee_fndecl); > + break; > + case DIRS_READ: > + inform (DECL_SOURCE_LOCATION (m_callee_fndecl), > + "argument %d of %qD must be a write-only file descriptor", > + m_arg_idx + 1, m_callee_fndecl); > + break; > + } > + } I don't like the wording of the direction-based messages; if I'm following the code correctly, it's not so much that the argument must be, say, a read-only file descriptor, as that the argument must be a *readable* file descriptor. For example in fd-5.c test_2 you have: void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message "argument 1 of 'g' must be a read-only file descriptor" } */ void test_2 (const char *path) { int fd = open (path, O_WRONLY); if (fd != -1) { g (fd); /* { dg-warning "'g' on 'write-only' file descriptor 'fd'" } */ } close (fd); } so presumably with the patch as posted it emits: warning: 'g' on 'write-only' file descriptor 'fd' note: argument 1 of 'g' must be a read-only file descriptor whereas I think we really mean: warning: 'g' on write-only file descriptor 'fd' note: argument 1 of 'g' must be a readable file descriptor Also, it will be easier for the user to understand why these warnings are appearing if they refer to the attribute by name. So please add something like: "due to %<__attribute__((fd_arg(%d)))%>", m_arg_idx + 1 to the format strings and arguments. So the example above might read: warning: 'g' on write-only file descriptor 'fd' note: argument 1 of 'g' must be a readable file descriptor, due to '__attribute__((fd_arg_read(1)))' [...snip...] > @@ -317,29 +398,25 @@ public: > bool > emit (rich_location *rich_loc) final override > { > + bool warned; > switch (m_fd_dir) > { > - case DIR_READ: > - return warning_at (rich_loc, get_controlling_option (), > + case DIRS_READ: > + warned = warning_at (rich_loc, get_controlling_option (), > "%qE on % file descriptor %qE", I hadn't noticed this before, but read-only shouldn't be in quotes in this message. > m_callee_fndecl, m_arg); > - case DIR_WRITE: > - return warning_at (rich_loc, get_controlling_option (), > + break; > + case DIRS_WRITE: > + warned = warning_at (rich_loc, get_controlling_option (), > "%qE on % file descriptor %qE", Likewise. > m_callee_fndecl, m_arg); > + break; > default: > gcc_unreachable (); > } > - } > - > - bool > - subclass_equal_p (const pending_diagnostic &base_other) const override > - { > - const fd_access_mode_mismatch &sub_other > - = (const fd_access_mode_mismatch &)base_other; > - return (same_tree_p (m_arg, sub_other.m_arg) > - && m_callee_fndecl == sub_other.m_callee_fndecl > - && m_fd_dir == sub_other.m_fd_dir); > + if (warned) > + inform_filedescriptor_attribute (m_fd_dir); > + return warned; > } > > label_text > @@ -347,10 +424,10 @@ public: > { > switch (m_fd_dir) > { > - case DIR_READ: > + case DIRS_READ: > return ev.formatted_print ("%qE on % file descriptor %qE", > m_callee_fndecl, m_arg); Likewise. > - case DIR_WRITE: > + case DIRS_WRITE: > return ev.formatted_print ("%qE on % file descriptor %qE", > m_callee_fndecl, m_arg); Likewise. [...snip...] > @@ -542,7 +627,7 @@ public: > > private: > diagnostic_event_id_t m_first_open_event; > - const tree m_callee_fndecl; > + We can lose this extra newline. [...snip...] > +void > +fd_state_machine::check_for_fd_attrs ( > + sm_context *sm_ctxt, const supernode *node, const gimple *stmt, > + const tree callee_fndecl, const char *attr_name, > + access_directions fd_attr_access_dir) const > +{ > + > + tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl)); > + attrs = lookup_attribute (attr_name, attrs); > + if (!attrs) > + return; > + > + if (!TREE_VALUE (attrs)) > + return; > + > + auto_bitmap argmap; > + > + for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx)) > + { > + unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1; > + bitmap_set_bit (argmap, val); > + } > + if (bitmap_empty_p (argmap)) > + return; > + > + for (unsigned i = 0; i < gimple_call_num_args (stmt); i++) > + { > + > + unsigned int arg_idx = i; I think "i" is redundant here. Just declare and use "arg_idx" directly in the "for" loop; it's much more descriptive than just "i". [...snip...] > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc > index c8d96723f4c..a04cc541f95 100644 > --- a/gcc/c-family/c-attribs.cc > +++ b/gcc/c-family/c-attribs.cc [...snip...] > @@ -426,7 +427,7 @@ const struct attribute_spec c_common_attribute_table[] = > { "tls_model", 1, 1, true, false, false, false, > handle_tls_model_attribute, NULL }, > { "nonnull", 0, -1, false, true, true, false, > - handle_nonnull_attribute, NULL }, > + handle_nonnull_attribute, NULL }, There's a stray addition of extra whitespace at the end of this line; please fix to avoid noise in the git history. [...snip...] Thanks again for the patch; hope the above make sense Dave