Re: [PATCH] Adding three new function attributes for static analysis of file descriptors

[David Malcolm <dmalcolm@redhat.com>]

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 <mirimmad@outlook.com>

[...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 %<read-only%> 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 %<write-only%> 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 %<read-only%> file descriptor %qE",
>                                     m_callee_fndecl, m_arg);

Likewise.

> -      case DIR_WRITE:
> +      case DIRS_WRITE:
>          return ev.formatted_print ("%qE on %<write-only%> 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