This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] fold constant flexarrays to strings (PR 91490)


On 8/21/19 2:50 PM, Martin Sebor wrote:
> This patch is a subset of the solution for PR 91457 whose main
> goal is to eliminate inconsistencies in warnings issued for
> out-of-bounds accesses to the various flavors of flexible array
> members of constant objects.  That patch was posted here:
>   https://gcc.gnu.org/ml/gcc-patches/2019-08/msg01202.html
> 
> Like PR 91457, this patch also relies on improving optimization
> to issue better quality warnings.  (I.e., with the latter being
> what motivated it.)
> 
> The optimization enhances string_constant to recognize empty
> CONSTRUCTORs returned by fold_ctor_reference for references
> to members of constant objects with either "insufficient"
> initializers (e.g., given struct S { char n, a[]; } s = { 0 };)
> or with braced-list initializers (e.g., given
> struct S s = { 3 { 1, 2, 3, 0 } };  The patch lets string_constant
> convert the CONSTRUCTOR for s.a into a STRING_CST, which in turn
> enables the folding of calls to built-ins like strlen, strchr, or
> strcmp with such arguments.
> 
> Exposing the strings to the folder then also lets it detect and
> issue warnings for out-of-bounds offsets in more instances of
> such references than before.
> 
> The remaining changes in the patch mostly enhance the places
> that use the no-warning bit to prevent redundant diagnostics.
> 
> Tested on x86_64-linux.
> 
> Martin
> 
> gcc-91490.diff
> 
> PR middle-end/91490 - bogus argument missing terminating nul warning on strlen of a flexible array member
> 
> gcc/c-family/ChangeLog:
> 
> 	PR middle-end/91490
> 	* c-common.c (braced_list_to_string): Add argument and overload.
> 	Handle flexible length arrays.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR middle-end/91490
> 	* c-c++-common/Warray-bounds-7.c: New test.
> 	* gcc.dg/Warray-bounds-39.c: Expect either -Warray-bounds or
> 	-Wstringop-overflow.
> 	* gcc.dg/strlenopt-78.c: New test.
> 
> gcc/ChangeLog:
> 
> 	PR middle-end/91490
> 	* builtins.c (c_strlen): Rename argument and introduce new local.
> 	Set no-warning bit on original argument.
> 	* expr.c (string_constant): Pass argument type to fold_ctor_reference.
> 	* gimple-fold.c (fold_nonarray_ctor_reference): Return a STRING_CST
> 	for missing initializers.
> 	* tree.c (build_string_literal): Handle optional argument.
> 	* tree.h (build_string_literal): Add defaulted argument.
> 	* gimple-ssa-warn-restrict.c (maybe_diag_access_bounds): Check
> 	no-warning bit on original expression.
> 

> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 2810867235d..ef942ffce57 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -8868,7 +8876,7 @@ braced_lists_to_strings (tree type, tree ctor)
>        unsigned HOST_WIDE_INT idx;
>        FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (ctor), idx, val)
>  	{
> -	  val = braced_lists_to_strings (ttp, val);
> +	  val = braced_lists_to_strings (ttp, val, code == RECORD_TYPE);
Do you need to handle UNION_TYPE or QUAL_UNION_TYPE here too?  If so,
RECORD_OR_UNION_TYPE_P is a better test.


> diff --git a/gcc/expr.c b/gcc/expr.c
> index 92979289e83..d16e0982dcf 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -11402,6 +11402,16 @@ is_aligning_offset (const_tree offset, const_tree exp)
>  tree
>  string_constant (tree arg, tree *ptr_offset, tree *mem_size, tree *decl)
>  {
> +  tree dummy = NULL_TREE;;
> +  if (!mem_size)
> +    mem_size = &dummy;
> +
> +  tree chartype = TREE_TYPE (arg);
> +  if (POINTER_TYPE_P (chartype))
> +    chartype = TREE_TYPE (chartype);
> +  while (TREE_CODE (chartype) == ARRAY_TYPE)
> +    chartype = TREE_TYPE (chartype);
I'm hesitant to ACK this bit.  I can understand that if we're given an
ADDR_EXPR, which should be a POINTER_TYPE_P, that we'll want to look at
what it's pointing to.  But shouldn't we verify that it's an ADDR_EXPR
before just blindly stripping away the outer type?

I also wonder if this (assuming we keep it in some form) belongs after
the STRIP_NOPs.


> @@ -11602,17 +11605,39 @@ string_constant (tree arg, tree *ptr_offset, tree *mem_size, tree *decl)
>        int len = native_encode_expr (init, charbuf, sizeof charbuf, 0);
>        if (len > 0)
>  	{
> -	  /* Construct a string literal with elements of ELTYPE and
> +	  /* Construct a string literal with elements of INITTYPE and
>  	     the representation above.  Then strip
>  	     the ADDR_EXPR (ARRAY_REF (...)) around the STRING_CST.  */
> -	  init = build_string_literal (len, (char *)charbuf, eltype);
> +	  init = build_string_literal (len, (char *)charbuf, inittype);
>  	  init = TREE_OPERAND (TREE_OPERAND (init, 0), 0);
>  	}
>      }
>  
> +  tree initsize = TYPE_SIZE_UNIT (inittype);
> +
> +  if (TREE_CODE (init) == CONSTRUCTOR && !CONSTRUCTOR_ELTS (init))
Would initializer_zerop be better here?  That would catch
zero-initialized constructors as well.

If not, then you want to make sure to check !TREE_CLOBBER_P (init) since
that's something completely different than zero initialization.


>  
>    return init;
> diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
> index d576b0842f9..fcffb9802b7 100644
> --- a/gcc/gimple-fold.c
> +++ b/gcc/gimple-fold.c
[ ... ]
None of the gimple-fold stuff looked strictly necessary for this patch.
  But gimple-fold changes are OK.


I think you need to double-check your ChangeLog as well.  There's
changes in expr.c that aren't reflected in the ChangeLog.  The changes
to fold_nonarray_ctor_reference don't seem to match the ChangeLog either.

Mostly this looks OK.  I think we close on the issues above and this
will be ready for the trunk.

jeff



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]