public inbox for gcc@gcc.gnu.org
 help / color / mirror / Atom feed
From: David Malcolm <dmalcolm@redhat.com>
To: Mir Immad <mirimnan017@gmail.com>
Cc: gcc@gcc.gnu.org, libc-alpha@sourceware.org
Subject: Re: Adding file descriptor attribute(s) to gcc and glibc
Date: Tue, 12 Jul 2022 18:25:42 -0400	[thread overview]
Message-ID: <5769682d0d17579cbd72f72a4001bfa8444b80a8.camel@redhat.com> (raw)
In-Reply-To: <260f0b41c663133cea96eb64bd85e8ba16d8a936.camel@redhat.com>

On Tue, 2022-07-12 at 18:16 -0400, David Malcolm wrote:
> On Tue, 2022-07-12 at 23:03 +0530, Mir Immad wrote:
> 
> [cross-posting to the glibc development mailing list; updating subject
> accordingly]
> 
> > Hi everyone,
> 
> Hi Immad, GCC developers, and glibc developers.
> 
> glibc developers: Immad is a GSoC student, he's been adding checks for
> file-descriptor-based APIs to gcc's -fanalyzer:
>   
> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=97baacba963c06e3d0e33cde04e7e687671e60e7
> 
> > 
> > This is an in-progress patch for adding three new function attributes
> > to
> > GCC for static analysis of file descriptor APIs (which is a part of
> > my
> > GSoC
> > project)
> 
> As you say, the patch is "in-progress".
> 
> Immad and I discussed this off-list and considered a few different
> ideas about this; I'm especially hoping for feedback from glibc
> developers on this idea, as I think the attributes could potentially be
> used in dozens of places in glibc's headers.
> 
> > 
> > 1)  __attribute__((accesses_fd(N)))
> > This attribute expected argument N of a function to be an open file
> > descriptor. (see test_1 of fd-5.c)
> 
> The patch is missing a few things, of which I think the most important
> for a discussion is documentation: what do we want these attributes to
> mean?
> 
> Presumably: __attribute__((accesses_fd(N))) means something like:
> 
> This function attribute documents to human and automated readers of the
> code that the function expects the argument with the given index to be
> a valid, open file-descriptor.
> 
> If -fanalyzer is enabled, it will complain with:
> * -Wanalyzer-fd-use-without-check if the given argument is the result
> of an "open" call that hasn't yet been checked for validity
> * -Wanalyzer-fd-use-after-close if the given argument is a closed FD
> 
> The argument must be of "int" type.
> 
> 
> ...or somesuch.
> 
> > 
> > 2) __attribute__((reads_from_fd(N)))
> > This attribute expects arguments N of a function to not be a write-
> > only
> > FD.
> > (see test_2 of fd-5.c)
> 
> Maybe reword to: this is equivalent to "accesses_fd", but with the
> additional check that the file descriptor must not have been opened
> write-only; the analyzer will complain with -Wanalyzer-fd-access-mode-
> mismatch if this is the case.
> 
> 
> > 
> > 3) __attribute__((writes_to_fd(N)))
> > This attribute expects arguments N of a function to not be a read-
> > only
> > FD.
> > (see test_3 of fd-6.c)
> 
> As above, but must not have been opened read-only.
> 
> 
> FWIW I'm not sure whether I prefer having three attributes, or one
> attribute with an optional access direction parameter.
> 
> One drawback of the "three attributes" approach is that they're
> alphabetically distant from each other, obscuring their close
> relationship.
> 
> GCC's attribute syntax here:
>   https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> allows for a parenthesized list of parameters for the attribute, which
> can be:
>  (a) An identifier
>  (b) An identifier followed by a comma and a non-empty comma-separated
> list of expressions
>  (c) A possibly empty comma-separated list of expressions
> 
> I'd hoped to have an argument number, with an optional extra param
> describing the direction of the access, but syntax (b) puts the
> identifier first, alas.
> 
> Here's one possible way of doing it with a single attribute, via syntax
> (b):
> e.g.
>    __attribute__((fd_argument (access, 1))
>    __attribute__((fd_argument (read, 1))
>    __attribute__((fd_argument (write, 1))
> 
> meaning that argument 1 of the function is expected to be an open file-
> descriptor, and that it must be possible to read from/write to that fd
> for cases 2 and 3.
> 
> Here are some possible examples of how glibc might use this syntax:
> 
>     int dup (int oldfd)
>       __attribute((fd_argument (access, 1)); 
> 
>     int ftruncate (int fd, off_t length)
>       __attribute((fd_argument (access, 1)); 
> 
>     ssize_t pread(int fd, void *buf, size_t count, off_t offset)
>       __attribute((fd_argument (read, 1));
> 
>     ssize_t pwrite(int fd, const void *buf, size_t count, 
>                    off_t offset);
>       __attribute((fd_argument (write, 1));
> 
> ...but as I said, I'm most interested in input from glibc developers on
> this.

I just realized that the attribute could accept both the single integer
argument number (syntax (c)) for the "don't care about access
direction" case, or the ({read|write}, N) of syntax (b) above, giving
e.g.:

    int dup (int oldfd)
      __attribute((fd_argument (1)); 

    int ftruncate (int fd, off_t length)
      __attribute((fd_argument (1)); 

    ssize_t pread(int fd, void *buf, size_t count, off_t offset)
      __attribute((fd_argument (read, 1));

    ssize_t pwrite(int fd, const void *buf, size_t count, 
                   off_t offset);
      __attribute((fd_argument (write, 1));

for the above examples.

How does that look?
Dave

> 
> Hope this is constructive
> Dave
> 
> > 
> > Thanks.
> > Immad
> > 
> > 
> > 
> > 
> > 
> > On Tue, Jul 12, 2022 at 11:02 PM Immad Mir <mirimmad@outlook.com>
> > wrote:
> > 
> > > ---
> > >  gcc/analyzer/sm-fd.cc                | 231
> > > ++++++++++++++++++++++++-
> > > --
> > >  gcc/c-family/c-attribs.cc            |  63 ++++++++
> > >  gcc/testsuite/gcc.dg/analyzer/fd-5.c |  44 +++++
> > >  gcc/testsuite/gcc.dg/analyzer/fd-6.c |   4 +
> > >  4 files changed, 322 insertions(+), 20 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > >  create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c
> > > 
> > > diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> > > index 8e4300b06e2..bdc807160cc 100644
> > > --- a/gcc/analyzer/sm-fd.cc
> > > +++ b/gcc/analyzer/sm-fd.cc
> > > @@ -39,10 +39,13 @@ along with GCC; see the file COPYING3.  If not
> > > see
> > >  #include "analyzer/analyzer-selftests.h"
> > >  #include "tristate.h"
> > >  #include "selftest.h"
> > > +#include "stringpool.h"
> > > +#include "attribs.h"
> > >  #include "analyzer/call-string.h"
> > >  #include "analyzer/program-point.h"
> > >  #include "analyzer/store.h"
> > >  #include "analyzer/region-model.h"
> > > +#include "bitmap.h"
> > > 
> > >  #if ENABLE_ANALYZER
> > > 
> > > @@ -59,6 +62,13 @@ enum access_mode
> > >    WRITE_ONLY
> > >  };
> > > 
> > > +enum fd_access_direction
> > > +{
> > > +  DIR_READ_WRITE,
> > > +  DIR_READ,
> > > +  DIR_WRITE
> > > +};
> > > +
> > >  class fd_state_machine : public state_machine
> > >  {
> > >  public:
> > > @@ -104,6 +114,8 @@ public:
> > >    bool is_readonly_fd_p (state_t s) const;
> > >    bool is_writeonly_fd_p (state_t s) const;
> > >    enum access_mode get_access_mode_from_flag (int flag) const;
> > > +  void inform_filedescriptor_attribute (tree callee_fndecl, int
> > > arg,
> > > +                                        fd_access_direction
> > > fd_dir)
> > > const;
> > > 
> > >    /* State for a constant file descriptor (>= 0) */
> > >    state_t m_constant_fd;
> > > @@ -146,7 +158,7 @@ private:
> > >    void check_for_open_fd (sm_context *sm_ctxt, const supernode
> > > *node,
> > >                            const gimple *stmt, const gcall *call,
> > >                            const tree callee_fndecl,
> > > -                          enum access_direction access_fn) const;
> > > +                          enum fd_access_direction access_fn)
> > > const;
> > > 
> > >    void make_valid_transitions_on_condition (sm_context *sm_ctxt,
> > >                                              const supernode *node,
> > > @@ -156,6 +168,12 @@ private:
> > >                                                const supernode
> > > *node,
> > >                                                const gimple *stmt,
> > >                                                const svalue *lhs)
> > > const;
> > > +  bitmap get_fd_attrs (const char *attr_name, tree callee_fndecl)
> > > const;
> > > +  void check_for_fd_attrs (sm_context *sm_ctxt, const supernode
> > > *node,
> > > +                          const gimple *stmt, const gcall *call,
> > > +                          const tree callee_fndecl, bitmap argmap,
> > > +                          fd_access_direction fd_attr_access_dir)
> > > const;
> > > +
> > >  };
> > > 
> > >  /* Base diagnostic class relative to fd_state_machine. */
> > > @@ -294,10 +312,10 @@ class fd_access_mode_mismatch : public
> > > fd_diagnostic
> > >  {
> > >  public:
> > >    fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> > > -                           enum access_direction fd_dir,
> > > -                           const tree callee_fndecl)
> > > +                           enum fd_access_direction fd_dir,
> > > +                           const tree callee_fndecl, bool attr,
> > > int
> > > arg_idx)
> > >        : fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
> > > -        m_callee_fndecl (callee_fndecl)
> > > +        m_callee_fndecl (callee_fndecl), m_attr (attr), m_arg_idx
> > > (arg_idx)
> > > 
> > >    {
> > >    }
> > > @@ -317,19 +335,27 @@ 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 (),
> > > +        warned =  warning_at (rich_loc, get_controlling_option (),
> > >                             "%qE on %<read-only%> file descriptor
> > > %qE",
> > >                             m_callee_fndecl, m_arg);
> > > +        break;
> > >        case DIR_WRITE:
> > > -        return warning_at (rich_loc, get_controlling_option (),
> > > +        warned = warning_at (rich_loc, get_controlling_option (),
> > >                             "%qE on %<write-only%> file descriptor
> > > %qE",
> > >                             m_callee_fndecl, m_arg);
> > > +        break;
> > >        default:
> > >          gcc_unreachable ();
> > >        }
> > > +      if (warned && m_attr)
> > > +      {
> > > +        m_sm.inform_filedescriptor_attribute (m_callee_fndecl,
> > > m_arg_idx,
> > > m_fd_dir);
> > > +      }
> > > +      return warned;
> > >    }
> > > 
> > >    bool
> > > @@ -359,8 +385,10 @@ public:
> > >    }
> > > 
> > >  private:
> > > -  enum access_direction m_fd_dir;
> > > +  enum fd_access_direction m_fd_dir;
> > >    const tree m_callee_fndecl;
> > > +  bool m_attr;
> > > +  int m_arg_idx;
> > >  };
> > > 
> > >  class double_close : public fd_diagnostic
> > > @@ -422,8 +450,9 @@ class fd_use_after_close : public fd_diagnostic
> > >  {
> > >  public:
> > >    fd_use_after_close (const fd_state_machine &sm, tree arg,
> > > -                      const tree callee_fndecl)
> > > -      : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
> > > +                      const tree callee_fndecl, bool attr, int
> > > arg_idx)
> > > +      : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
> > > m_attr
> > > (attr),
> > > +        m_arg_idx (arg_idx)
> > >    {
> > >    }
> > > 
> > > @@ -439,12 +468,27 @@ public:
> > >      return OPT_Wanalyzer_fd_use_after_close;
> > >    }
> > > 
> > > +  bool
> > > +  subclass_equal_p (const pending_diagnostic &base_other) const
> > > override
> > > +  {
> > > +    const fd_use_after_close &sub_other
> > > +        = (const fd_use_after_close &)base_other;
> > > +    return (same_tree_p (m_arg, sub_other.m_arg)
> > > +            && m_callee_fndecl == sub_other.m_callee_fndecl
> > > +            && m_attr == sub_other.m_attr
> > > +            && m_arg_idx == sub_other.m_arg_idx);
> > > +  }
> > > +
> > >    bool
> > >    emit (rich_location *rich_loc) final override
> > >    {
> > > -    return warning_at (rich_loc, get_controlling_option (),
> > > +    bool warned;
> > > +    warned = warning_at (rich_loc, get_controlling_option (),
> > >                         "%qE on closed file descriptor %qE",
> > > m_callee_fndecl,
> > >                         m_arg);
> > > +    if (warned && m_attr)
> > > +      m_sm.inform_filedescriptor_attribute (m_callee_fndecl,
> > > m_arg_idx,
> > > DIR_READ_WRITE);
> > > +    return warned;
> > >    }
> > > 
> > >    label_text
> > > @@ -466,17 +510,19 @@ public:
> > >    describe_final_event (const evdesc::final_event &ev) final
> > > override
> > >    {
> > >      if (m_first_close_event.known_p ())
> > > -      return ev.formatted_print (
> > > -          "%qE on closed file descriptor %qE; %qs was at %@",
> > > m_callee_fndecl,
> > > -          m_arg, "close", &m_first_close_event);
> > > -    else
> > > -      return ev.formatted_print ("%qE on closed file descriptor
> > > %qE",
> > > -                                 m_callee_fndecl, m_arg);
> > > +        return ev.formatted_print (
> > > +            "%qE on closed file descriptor %qE; %qs was at %@",
> > > m_callee_fndecl,
> > > +            m_arg, "close", &m_first_close_event);
> > > +      else
> > > +        return ev.formatted_print ("%qE on closed file descriptor
> > > %qE",
> > > +                                  m_callee_fndecl, m_arg);
> > >    }
> > > 
> > >  private:
> > >    diagnostic_event_id_t m_first_close_event;
> > >    const tree m_callee_fndecl;
> > > +  bool m_attr;
> > > +  int m_arg_idx;
> > >  };
> > > 
> > >  class unchecked_use_of_fd : public fd_diagnostic
> > > @@ -543,8 +589,38 @@ public:
> > >  private:
> > >    diagnostic_event_id_t m_first_open_event;
> > >    const tree m_callee_fndecl;
> > > +
> > >  };
> > > 
> > > +/* Issue a note regarding misuse of a file descriptor.
> > > +  ARG_IDX here is 0-based.
> > > + */
> > > +void
> > > +fd_state_machine::inform_filedescriptor_attribute (
> > > +    tree callee_fndecl, int arg_idx, fd_access_direction fd_dir)
> > > const
> > > +{
> > > +  switch (fd_dir)
> > > +    {
> > > +    case READ_WRITE:
> > > +      inform (DECL_SOURCE_LOCATION (callee_fndecl),
> > > +              "argument %d of %qD must be an open file
> > > descriptor",
> > > arg_idx + 1,
> > > +              callee_fndecl);
> > > +      break;
> > > +    case DIR_WRITE:
> > > +      inform (DECL_SOURCE_LOCATION (callee_fndecl),
> > > +              "argument %d of %qD must be a read-only file
> > > descriptor",
> > > +              arg_idx + 1, callee_fndecl);
> > > +      break;
> > > +    case DIR_READ:
> > > +      inform (DECL_SOURCE_LOCATION (callee_fndecl),
> > > +              "argument %d of %qD must be a write-only file
> > > descriptor",
> > > +              arg_idx + 1, callee_fndecl);
> > > +      break;
> > > +    }
> > > +}
> > > +
> > > +
> > > +
> > >  fd_state_machine::fd_state_machine (logger *logger)
> > >      : state_machine ("file-descriptor", logger),
> > >        m_constant_fd (add_state ("fd-constant")),
> > > @@ -647,11 +723,124 @@ fd_state_machine::on_stmt (sm_context
> > > *sm_ctxt,
> > > const supernode *node,
> > >              on_read (sm_ctxt, node, stmt, call, callee_fndecl);
> > >              return true;
> > >            } // "read"
> > > +
> > > +          // Handle __attribute__((filedescriptor))
> > > +          {
> > > +            bitmap argmap = get_fd_attrs ("accesses_fd",
> > > callee_fndecl);
> > > +            if (argmap)
> > > +              check_for_fd_attrs (sm_ctxt, node, stmt, call,
> > > callee_fndecl, argmap, DIR_READ_WRITE);
> > > +
> > > +            bitmap read_argmap = get_fd_attrs ("reads_from_fd",
> > > callee_fndecl);
> > > +            if(read_argmap)
> > > +              check_for_fd_attrs (sm_ctxt, node, stmt, call,
> > > callee_fndecl, read_argmap, DIR_READ);
> > > +
> > > +            bitmap write_argmap = get_fd_attrs ("writes_to_fd",
> > > callee_fndecl);
> > > +            if (write_argmap)
> > > +              check_for_fd_attrs (sm_ctxt, node, stmt, call,
> > > callee_fndecl, write_argmap, DIR_WRITE);
> > > +
> > > +            return true;
> > > +          }
> > > +
> > >        }
> > > 
> > >    return false;
> > >  }
> > > 
> > > +void
> > > +fd_state_machine::check_for_fd_attrs (sm_context *sm_ctxt,
> > > +                                      const supernode *node, const
> > > gimple
> > > *stmt,
> > > +                                      const gcall *call,
> > > +                                      const tree callee_fndecl,
> > > bitmap
> > > argmap,
> > > +                                      fd_access_direction
> > > fd_attr_access_dir) const
> > > +{
> > > +  for (unsigned i = 0; i < gimple_call_num_args (stmt); i++)
> > > +    {
> > > +      unsigned int arg_idx = i;
> > > +      tree arg = gimple_call_arg (stmt, i);
> > > +      tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > > +      state_t state = sm_ctxt->get_state (stmt, arg);
> > > +      if (TREE_CODE (TREE_TYPE (arg)) != INTEGER_TYPE)
> > > +        continue;
> > > +      if (bitmap_bit_p (argmap, arg_idx)) // check if the arg is
> > > any
> > > of
> > > the file
> > > +                                          // descriptor attributes
> > > +        {
> > > +
> > > +          if (is_closed_fd_p (state))
> > > +            {
> > > +
> > > +              sm_ctxt->warn (node, stmt, arg,
> > > +                             new fd_use_after_close (*this,
> > > diag_arg,
> > > +                                                    
> > > callee_fndecl,
> > > true,
> > > +                                                     arg_idx));
> > > +              continue;
> > > +            }
> > > +          else
> > > +            {
> > > +              if (!(is_valid_fd_p (state) || (state == m_stop)))
> > > +                {
> > > +                  if (!is_constant_fd_p (state))
> > > +                    sm_ctxt->warn (node, stmt, arg,
> > > +                                   new unchecked_use_of_fd (*this,
> > > diag_arg,
> > > +
> > > callee_fndecl));
> > > +                }
> > > +            }
> > > +        }
> > > +      switch (fd_attr_access_dir)
> > > +        {
> > > +        case DIR_READ:
> > > +          if (bitmap_bit_p (argmap, arg_idx)) // check if the arg
> > > is
> > > marked by
> > > +                                              // reads_from_fd
> > > attribute
> > > +            {
> > > +              if (is_writeonly_fd_p (state))
> > > +                {
> > > +                  sm_ctxt->warn (node, stmt, arg,
> > > +                                 new fd_access_mode_mismatch
> > > (*this,
> > > diag_arg,
> > > +                                                             
> > > DIR_WRITE,
> > > +
> > > callee_fndecl, true, i));
> > > +                }
> > > +            }
> > > +          break;
> > > +        case DIR_WRITE:
> > > +          if (bitmap_bit_p (argmap, arg_idx)) // check if the arg
> > > is
> > > marked by
> > > +                                              // writes_to_fd
> > > attribute
> > > +            {
> > > +              if (is_readonly_fd_p (state))
> > > +                {
> > > +                  sm_ctxt->warn (node, stmt, arg,
> > > +                                 new fd_access_mode_mismatch (
> > > +                                     *this, diag_arg, DIR_READ,
> > > callee_fndecl, true, i));
> > > +                }
> > > +            }
> > > +          break;
> > > +        default:
> > > +          gcc_unreachable ();
> > > +        }
> > > +    }
> > > +}
> > > +
> > > +bitmap
> > > +fd_state_machine::get_fd_attrs (const char *attr_name, tree
> > > callee_fndecl) const
> > > +{
> > > +  bitmap argmap = NULL;
> > > +  tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
> > > +  attrs = lookup_attribute (attr_name, attrs);
> > > +  if (!attrs)
> > > +    return argmap;
> > > +
> > > +  if (!TREE_VALUE (attrs))
> > > +    return argmap;
> > > +
> > > +  argmap = BITMAP_ALLOC (NULL);
> > > +
> > > +  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);
> > > +    }
> > > +  return argmap;
> > > +}
> > > +
> > > +
> > >  void
> > >  fd_state_machine::on_open (sm_context *sm_ctxt, const supernode
> > > *node,
> > >                             const gimple *stmt, const gcall *call)
> > > const
> > > @@ -729,7 +918,7 @@ void
> > >  fd_state_machine::check_for_open_fd (
> > >      sm_context *sm_ctxt, const supernode *node, const gimple
> > > *stmt,
> > >      const gcall *call, const tree callee_fndecl,
> > > -    enum access_direction callee_fndecl_dir) const
> > > +    enum fd_access_direction callee_fndecl_dir) const
> > >  {
> > >    tree arg = gimple_call_arg (call, 0);
> > >    tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > > @@ -738,7 +927,7 @@ fd_state_machine::check_for_open_fd (
> > >    if (is_closed_fd_p (state))
> > >      {
> > >        sm_ctxt->warn (node, stmt, arg,
> > > -                     new fd_use_after_close (*this, diag_arg,
> > > callee_fndecl));
> > > +                     new fd_use_after_close (*this, diag_arg,
> > > callee_fndecl, false, -1));
> > >      }
> > > 
> > >    else
> > > @@ -758,7 +947,7 @@ fd_state_machine::check_for_open_fd (
> > >                tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > >                sm_ctxt->warn (node, stmt, arg,
> > >                               new fd_access_mode_mismatch (
> > > -                                 *this, diag_arg, DIR_WRITE,
> > > callee_fndecl));
> > > +                                 *this, diag_arg, DIR_WRITE,
> > > callee_fndecl, false, -1));
> > >              }
> > > 
> > >            break;
> > > @@ -769,9 +958,11 @@ fd_state_machine::check_for_open_fd (
> > >                tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > >                sm_ctxt->warn (node, stmt, arg,
> > >                               new fd_access_mode_mismatch (
> > > -                                 *this, diag_arg, DIR_READ,
> > > callee_fndecl));
> > > +                                 *this, diag_arg, DIR_READ,
> > > callee_fndecl, false, -1));
> > >              }
> > >            break;
> > > +        default:
> > > +          gcc_unreachable ();
> > >          }
> > >      }
> > >  }
> > > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> > > index c8d96723f4c..84f29987bc6 100644
> > > --- a/gcc/c-family/c-attribs.cc
> > > +++ b/gcc/c-family/c-attribs.cc
> > > @@ -173,6 +173,10 @@ static tree handle_objc_nullability_attribute
> > > (tree
> > > *, tree, tree, int, bool *);
> > >  static tree handle_signed_bool_precision_attribute (tree *, tree,
> > > tree,
> > > int,
> > >                                                     bool *);
> > >  static tree handle_retain_attribute (tree *, tree, tree, int, bool
> > > *);
> > > +static tree handle_accesses_fd_attribute (tree *, tree, tree, int,
> > > bool
> > > *);
> > > +static tree handle_reads_from_fd_attribute (tree *, tree, tree,
> > > int,
> > > bool
> > > *);
> > > +static tree handle_writes_to_fd_attribute (tree *, tree, tree,
> > > int,
> > > bool
> > > *);
> > > +
> > > 
> > >  /* Helper to define attribute exclusions.  */
> > >  #define ATTR_EXCL(name, function, type, variable)      \
> > > @@ -427,6 +431,12 @@ const struct attribute_spec
> > > c_common_attribute_table[] =
> > >                               handle_tls_model_attribute, NULL },
> > >    { "nonnull",                0, -1, false, true, true, false,
> > >                               handle_nonnull_attribute, NULL },
> > > +  { "accesses_fd",  0, -1, false, true, true, false,
> > > +            handle_accesses_fd_attribute, NULL},
> > > +  { "reads_from_fd",  0, -1, false, true, true, false,
> > > +            handle_reads_from_fd_attribute, NULL},
> > > +  { "writes_to_fd",  0, -1, false, true, true, false,
> > > +            handle_writes_to_fd_attribute, NULL},
> > >    { "nonstring",              0, 0, true, false, false, false,
> > >                               handle_nonstring_attribute, NULL },
> > >    { "nothrow",                0, 0, true,  false, false, false,
> > > @@ -4521,6 +4531,59 @@ handle_nonnull_attribute (tree *node, tree
> > > name,
> > >    return NULL_TREE;
> > >  }
> > > 
> > > +/* Handle the "accesses_fd" attribute */
> > > +static tree
> > > +handle_accesses_fd_attribute (tree *node, tree name, tree args,
> > > +                              int ARG_UNUSED (flags), bool
> > > *no_add_attrs)
> > > +{
> > > +  tree type = *node;
> > > +  if (!args)
> > > +    {
> > > +      if (!prototype_p (type))
> > > +        {
> > > +          error ("%qE attribute without arguments on a non-
> > > prototype",
> > > name);
> > > +          *no_add_attrs = true;
> > > +        }
> > > +      return NULL_TREE;
> > > +    }
> > > +
> > > +  for (int i = 1; args; ++i)
> > > +    {
> > > +      tree pos = TREE_VALUE (args);
> > > +      tree next = TREE_CHAIN (args);
> > > +      if (tree val = positional_argument (type, name, pos,
> > > INTEGER_TYPE,
> > > +                                          next || i > 1 ? i : 0))
> > > +        TREE_VALUE (args) = val;
> > > +      else
> > > +        {
> > > +          *no_add_attrs = true;
> > > +          break;
> > > +        }
> > > +      args = next;
> > > +    }
> > > +}
> > > +
> > > +
> > > +
> > > +static tree
> > > +handle_reads_from_fd_attribute (tree *node, tree name, tree
> > > ARG_UNUSED
> > > (args),
> > > +          int ARG_UNUSED (flags), bool *no_add_attrs)
> > > +{
> > > +  // TODO: valiadate usage of this attribute
> > > +  return NULL_TREE;
> > > +}
> > > +
> > > +static tree
> > > +handle_writes_to_fd_attribute (tree *node, tree name, tree
> > > ARG_UNUSED
> > > (args),
> > > +          int ARG_UNUSED (flags), bool *no_add_attrs)
> > > +{
> > > +  // TODO: validate usage of this attribute
> > > +  return NULL_TREE;
> > > +}
> > > +
> > > +
> > > +
> > > +
> > >  /* Handle the "nonstring" variable attribute.  */
> > > 
> > >  static tree
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > > b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > > new file mode 100644
> > > index 00000000000..e597ba2367a
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > > @@ -0,0 +1,44 @@
> > > +int open(const char *, int mode);
> > > +void close(int fd);
> > > +int write (int fd, void *buf, int nbytes);
> > > +int read (int fd, void *buf, int nbytes);
> > > +
> > > +#define O_RDONLY 0
> > > +#define O_WRONLY 1
> > > +#define O_RDWR 2
> > > +
> > > +void f (int fd) __attribute__((accesses_fd(1))); /* { dg-message
> > > "argument 1 of 'f' must be an open file descriptor" } */
> > > +
> > > +void
> > > +test_1 (const char *path)
> > > +{
> > > +    int fd = open (path, O_RDWR);
> > > +    close(fd);
> > > +    f(fd); /* { dg-warning "'f' on closed file descriptor 'fd'" }
> > > */
> > > +      /* { dg-message "\\(3\\) 'f' on closed file descriptor 'fd';
> > > 'close' was at \\(2\\)" "" { target *-*-* } .-1 } */
> > > +}
> > > +
> > > +void g (int fd) __attribute__((reads_from_fd(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);
> > > +}
> > > +
> > > +void h (int fd) __attribute__((writes_to_fd(1))); /* { dg-message
> > > "argument 1 of 'h' must be a write-only file descriptor" } */
> > > +void
> > > +test_3 (const char *path)
> > > +{
> > > +  int fd = open (path, O_RDONLY);
> > > +  if (fd != -1)
> > > +  {
> > > +    h (fd); /* { dg-warning "'h' on 'read-only' file descriptor
> > > 'fd'" } */
> > > +  }
> > > +  close(fd);
> > > +}
> > > \ No newline at end of file
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> > > b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> > > new file mode 100644
> > > index 00000000000..9c0b55086a9
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> > > @@ -0,0 +1,4 @@
> > > +
> > > +int not_a_fn __attribute__ ((accesses_fd(1))); /* { dg-warning
> > > "'accesses_fd' attribute only applies to function types" } */
> > > +
> > > +void f (char *p) __attribute__ ((accesses_fd(1))); /* { dg-warning
> > > "'accesses_fd' attribute argument value '1' refers to parameter
> > > type
> > > 'char
> > > \\\*'" } */
> > > \ No newline at end of file
> > > --
> > > 2.25.1
> > > 
> > > 
> 



  reply	other threads:[~2022-07-12 22:25 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-12 17:31 [PATCH] filedescriptor attribute Immad Mir
2022-07-12 17:33 ` Mir Immad
2022-07-12 22:16   ` Adding file descriptor attribute(s) to gcc and glibc David Malcolm
2022-07-12 22:25     ` David Malcolm [this message]
2022-07-13  8:37       ` Szabolcs Nagy
2022-07-13  8:46         ` Andreas Schwab
2022-07-13 12:05         ` Florian Weimer
2022-07-13 13:33           ` David Malcolm
2022-07-13 14:01             ` Florian Weimer
2022-07-13 16:55               ` David Malcolm
2022-07-14  8:30                 ` Szabolcs Nagy
2022-07-14 15:22                   ` David Malcolm
2022-07-14 17:07                     ` Paul Eggert
2022-07-13 16:56               ` Mir Immad
2022-07-13 19:29                 ` David Malcolm
2022-07-13 12:57         ` David Malcolm

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5769682d0d17579cbd72f72a4001bfa8444b80a8.camel@redhat.com \
    --to=dmalcolm@redhat.com \
    --cc=gcc@gcc.gnu.org \
    --cc=libc-alpha@sourceware.org \
    --cc=mirimnan017@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).