Re: [PATCH v3] add object access attributes (PR 83859)

[Jeff Law <law@redhat.com>]

On 11/21/19 10:11 AM, Martin Sebor wrote:
> Attached is another revision of this enhancement, this one
> incorporating Richard's request for a more efficient encoding
> of the attributes to enable faster parsing.  There is just one
> attribute called access, with the rest being arguments.  This
> is transformed to access (mode-string) where the mode-string
> is a STRING_CST describing the access mode (read/write/both)
> and the two positional arguments for the function type.
> 
> I have also removed the attributes from the built-in functions
> since they're not used for anything yet, and added more argument
> validation.
> 
> To test this a little more extensively, I annotated a few Glibc
> <unistd.h> functions with the new attribute and rebuilt it and
> its test suite.  That exposed a problem with function pointers
> not being handled correctly so I fixed that by letting
> the access attribute apply to function pointers (and function
> pointer types in general).
> 
> When this is finalized, if there's time I'm still hoping to get
> back to the parts of the patch that make use of the attribute
> for -Wunused and -Wuninitialized that I removed on Jeff's request
> for smaller, independent changes.
> 
> In GCC 11 I'd like to look into tying this attribute in with
> _FORTIFY_SOURCE.
> 
> Martin
> 
> gcc-83859.diff
> 
> PR middle-end/83859 - attributes to associate pointer arguments and sizes
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/83859
> 	* attribs.h (struct attr_access): New.
> 	* attribs.c (decl_attributes): Add an informational note.
> 	* builtins.c (check_access): Make extern.  Consistently set no-warning
> 	after issuing a warning.  Handle calls through function pointers.  Set
> 	no-warning.
> 	* builtins.h (check_access): Declare.
> 	* calls.c (rdwr_access_hash): New type.
> 	(rdwr_map): Same.
> 	(init_attr_rdwr_indices): New function.
> 	(maybe_warn_rdwr_sizes): Same.
> 	(initialize_argument_information): Call init_attr_rdwr_indices.
> 	Call maybe_warn_rdwr_sizes.
> 	* doc/extend.texi (attribute access): Document new attribute.
> 
> gcc/c-family/ChangeLog:
> 
> 	PR middle-end/83859
> 	* c-attribs.c (handle_access_attribute): New function.
> 	(c_common_attribute_table): Add new attribute.
> 	(get_argument_type): New function.
> 	(append_access_attrs): New function.
> 	(get_nonnull_operand): Rename...
> 	(get_attribute_operand): ...to this.
> 	* c-common.c (get_nonnull_operand): Rename...
> 	(get_attribute_operand): ...to this.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR middle-end/83859
> 	* c-c++-common/attr-nonstring-8.c: Adjust text of expected warning.
> 	* gcc.dg/Wstringop-overflow-22.c: New test.
> 	* gcc.dg/Wstringop-overflow-23.c: New test.
> 	* gcc.dg/attr-access-read-only.c: New test.
> 	* gcc.dg/attr-access-read-write.c: New test.
> 	* gcc.dg/attr-access-read-write-2.c: New test.
> 	* gcc.dg/attr-access-write-only.c: New test.
> 

> diff --git a/gcc/calls.c b/gcc/calls.c
> index 62921351b11..15627abbd0d 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -52,6 +52,8 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tree-ssa-strlen.h"
>  #include "intl.h"
>  #include "stringpool.h"
> +#include "hash-map.h"
> +#include "hash-traits.h"
>  #include "attribs.h"
>  #include "builtins.h"
>  #include "gimple-fold.h"
> @@ -1258,6 +1260,9 @@ alloc_max_size (void)
>  bool
>  get_size_range (tree exp, tree range[2], bool allow_zero /* = false */)
>  {
> +  if (!exp)
> +    return false;
> +
>    if (tree_fits_uhwi_p (exp))
>      {
>        /* EXP is a constant.  */
This change isn't mentioned anywhere in the ChangeLog.  If its
intentional, please mention it in the ChangeLog.  If it's not
intentional, then drop it :-)



> diff --git a/gcc/gimple.h b/gcc/gimple.h
> index 5a190b1714d..10223386d04 100644
> --- a/gcc/gimple.h
> +++ b/gcc/gimple.h
> @@ -24,6 +24,8 @@ along with GCC; see the file COPYING3.  If not see
>  
>  #include "tree-ssa-alias.h"
>  #include "gimple-expr.h"
> +#include "function.h"
> +#include "basic-block.h"
I thought I asked before, can these be moved into the .c files where
they're actually needed?

Otherwise it looks OK to me.

jeff