Re: [PATCH] Tweak array_at_struct_end_p

[Christophe Lyon]

Hi,


On 4 May 2017 at 11:07, Richard Biener  wrote:
>
> The following picks the changes suggested as followup for PR80533
> that do not cause the warning regression on accessing a [0] array.
>
> Additionally the patch removes the unnecessary allow_compref of the
> function.
>
> The question whether we want to allow an array to extend into
> padding still stands.  This patch allows it for C99 flex arrays
> (but not pre-C99 GNU extension [0] due to the above warning
> regression, also not for [1] or larger arrays we treat as flex arrays
> when we can't see an underlying decl).
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
>
> Richard.
>
> 2017-05-04  Richard Biener  
>
> * tree.c (array_at_struct_end_p): Handle arrays at struct
> end with flexarrays more conservatively.  Refactor and treat
> arrays of arrays or aggregates more strict.  Fix
> VIEW_CONVERT_EXPR handling.  Remove allow_compref argument.
> * tree.c (array_at_struct_end_p): Adjust prototype.
> * emit-rtl.c (set_mem_attributes_minus_bitpos): Adjust.
> * gimple-fold.c (get_range_strlen): Likewise.
> * tree-chkp.c (chkp_may_narrow_to_field): Likewise.
>

Since this patch was committed (r247581), I've noticed regressions
on arm-none-linux-gnueabihf:
  - PASS now FAIL [PASS => FAIL]:

  Executed from: gfortran.dg/dg.exp
gfortran.dg/alloc_comp_auto_array_2.f90   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  (test for
excess errors)
gfortran.dg/alloc_comp_auto_array_2.f90   -O3 -g  (test for excess errors)

Christophe


> Index: gcc/tree.c
> ===
> --- gcc/tree.c  (revision 247542)
> +++ gcc/tree.c  (working copy)
> @@ -13227,18 +13235,26 @@ array_ref_up_bound (tree exp)
>return NULL_TREE;
>  }
>
> -/* Returns true if REF is an array reference to an array at the end of
> -   a structure.  If this is the case, the array may be allocated larger
> -   than its upper bound implies.  When ALLOW_COMPREF is true considers
> -   REF when it's a COMPONENT_REF in addition ARRAY_REF and
> -   ARRAY_RANGE_REF.  */
> +/* Returns true if REF is an array reference or a component reference
> +   to an array at the end of a structure.
> +   If this is the case, the array may be allocated larger
> +   than its upper bound implies.  */
>
>  bool
> -array_at_struct_end_p (tree ref, bool allow_compref)
> +array_at_struct_end_p (tree ref)
>  {
> -  if (TREE_CODE (ref) != ARRAY_REF
> -  && TREE_CODE (ref) != ARRAY_RANGE_REF
> -  && (!allow_compref || TREE_CODE (ref) != COMPONENT_REF))
> +  tree atype;
> +
> +  if (TREE_CODE (ref) == ARRAY_REF
> +  || TREE_CODE (ref) == ARRAY_RANGE_REF)
> +{
> +  atype = TREE_TYPE (TREE_OPERAND (ref, 0));
> +  ref = TREE_OPERAND (ref, 0);
> +}
> +  else if (TREE_CODE (ref) == COMPONENT_REF
> +  && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 1))) == ARRAY_TYPE)
> +atype = TREE_TYPE (TREE_OPERAND (ref, 1));
> +  else
>  return false;
>
>while (handled_component_p (ref))
> @@ -13246,19 +13262,42 @@ array_at_struct_end_p (tree ref, bool al
>/* If the reference chain contains a component reference to a
>   non-union type and there follows another field the reference
>  is not at the end of a structure.  */
> -  if (TREE_CODE (ref) == COMPONENT_REF
> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE)
> +  if (TREE_CODE (ref) == COMPONENT_REF)
> {
> - tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1));
> - while (nextf && TREE_CODE (nextf) != FIELD_DECL)
> -   nextf = DECL_CHAIN (nextf);
> - if (nextf)
> -   return false;
> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE)
> +   {
> + tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1));
> + while (nextf && TREE_CODE (nextf) != FIELD_DECL)
> +   nextf = DECL_CHAIN (nextf);
> + if (nextf)
> +   return false;
> +   }
> }
> +  /* If we have a multi-dimensional array we do not consider
> + a non-innermost dimension as flex array if the whole
> +multi-dimensional array is at struct end.
> +Same for an array of aggregates with a trailing array
> +member.  */
> +  else if (TREE_CODE (ref) == ARRAY_REF)
> +   return false;
> +  else if (TREE_CODE (ref) == ARRAY_RANGE_REF)
> +   ;
> +  /* If we view an underlying object as sth else then what we
> + gathered up to now is what we have to rely on.  */
> +  else if (TREE_CODE (ref) == VIEW_CONVERT_EXPR)
> +   break;
> +  else
> +   gcc_unreachable ();
>
>ref = TREE_OPERAND (ref, 0);
>  }
>
> +  /* The array now is at struct end.  Treat flexible arrays as
> + always 

[PATCH] Tweak array_at_struct_end_p

[Richard Biener]

The following picks the changes suggested as followup for PR80533
that do not cause the warning regression on accessing a [0] array.

Additionally the patch removes the unnecessary allow_compref of the
function.

The question whether we want to allow an array to extend into
padding still stands.  This patch allows it for C99 flex arrays
(but not pre-C99 GNU extension [0] due to the above warning
regression, also not for [1] or larger arrays we treat as flex arrays
when we can't see an underlying decl).

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2017-05-04  Richard Biener  

* tree.c (array_at_struct_end_p): Handle arrays at struct
end with flexarrays more conservatively.  Refactor and treat
arrays of arrays or aggregates more strict.  Fix
VIEW_CONVERT_EXPR handling.  Remove allow_compref argument.
* tree.c (array_at_struct_end_p): Adjust prototype.
* emit-rtl.c (set_mem_attributes_minus_bitpos): Adjust.
* gimple-fold.c (get_range_strlen): Likewise.
* tree-chkp.c (chkp_may_narrow_to_field): Likewise.

Index: gcc/tree.c
===
--- gcc/tree.c  (revision 247542)
+++ gcc/tree.c  (working copy)
@@ -13227,18 +13235,26 @@ array_ref_up_bound (tree exp)
   return NULL_TREE;
 }
 
-/* Returns true if REF is an array reference to an array at the end of
-   a structure.  If this is the case, the array may be allocated larger
-   than its upper bound implies.  When ALLOW_COMPREF is true considers
-   REF when it's a COMPONENT_REF in addition ARRAY_REF and
-   ARRAY_RANGE_REF.  */
+/* Returns true if REF is an array reference or a component reference
+   to an array at the end of a structure.
+   If this is the case, the array may be allocated larger
+   than its upper bound implies.  */
 
 bool
-array_at_struct_end_p (tree ref, bool allow_compref)
+array_at_struct_end_p (tree ref)
 {
-  if (TREE_CODE (ref) != ARRAY_REF
-  && TREE_CODE (ref) != ARRAY_RANGE_REF
-  && (!allow_compref || TREE_CODE (ref) != COMPONENT_REF))
+  tree atype;
+
+  if (TREE_CODE (ref) == ARRAY_REF
+  || TREE_CODE (ref) == ARRAY_RANGE_REF)
+{
+  atype = TREE_TYPE (TREE_OPERAND (ref, 0));
+  ref = TREE_OPERAND (ref, 0);
+}
+  else if (TREE_CODE (ref) == COMPONENT_REF
+  && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 1))) == ARRAY_TYPE)
+atype = TREE_TYPE (TREE_OPERAND (ref, 1));
+  else
 return false;
 
   while (handled_component_p (ref))
@@ -13246,19 +13262,42 @@ array_at_struct_end_p (tree ref, bool al
   /* If the reference chain contains a component reference to a
  non-union type and there follows another field the reference
 is not at the end of a structure.  */
-  if (TREE_CODE (ref) == COMPONENT_REF
- && TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE)
+  if (TREE_CODE (ref) == COMPONENT_REF)
{
- tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1));
- while (nextf && TREE_CODE (nextf) != FIELD_DECL)
-   nextf = DECL_CHAIN (nextf);
- if (nextf)
-   return false;
+ if (TREE_CODE (TREE_TYPE (TREE_OPERAND (ref, 0))) == RECORD_TYPE)
+   {
+ tree nextf = DECL_CHAIN (TREE_OPERAND (ref, 1));
+ while (nextf && TREE_CODE (nextf) != FIELD_DECL)
+   nextf = DECL_CHAIN (nextf);
+ if (nextf)
+   return false;
+   }
}
+  /* If we have a multi-dimensional array we do not consider
+ a non-innermost dimension as flex array if the whole
+multi-dimensional array is at struct end.
+Same for an array of aggregates with a trailing array
+member.  */
+  else if (TREE_CODE (ref) == ARRAY_REF)
+   return false;
+  else if (TREE_CODE (ref) == ARRAY_RANGE_REF)
+   ;
+  /* If we view an underlying object as sth else then what we
+ gathered up to now is what we have to rely on.  */
+  else if (TREE_CODE (ref) == VIEW_CONVERT_EXPR)
+   break;
+  else
+   gcc_unreachable ();
 
   ref = TREE_OPERAND (ref, 0);
 }
 
+  /* The array now is at struct end.  Treat flexible arrays as
+ always subject to extend, even into just padding constrained by
+ an underlying decl.  */
+  if (! TYPE_SIZE (atype))
+return true;
+
   tree size = NULL;
 
   if (TREE_CODE (ref) == MEM_REF
Index: gcc/tree.h
===
--- gcc/tree.h  (revision 247542)
+++ gcc/tree.h  (working copy)
@@ -4885,12 +4885,10 @@ extern tree array_ref_up_bound (tree);
EXP, an ARRAY_REF or an ARRAY_RANGE_REF.  */
 extern tree array_ref_low_bound (tree);
 
-/* Returns true if REF is an array reference to an array at the end of
-   a structure.  If this is the case, the array may be allocated larger
-   than its upper bound implies.  When second argument is true