Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]

[Richard Biener <rguenther@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 19308 bytes --]

On Thu, 9 Mar 2023, Qing Zhao wrote:

> 
> 
> > On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@suse.de> wrote:
> > 
> > On Fri, 24 Feb 2023, Qing Zhao wrote:
> > 
> >> GCC extension accepts the case when a struct with a C99 flexible array member
> >> is embedded into another struct or union (possibly recursively).
> >> __builtin_object_size should treat such struct as flexible size.
> >> 
> >> gcc/c/ChangeLog:
> >> 
> >> 	PR tree-optimization/101832
> >> 	* c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
> >> 	struct/union type.
> > 
> > I can't really comment on the correctness of this part but since
> > only the C frontend will ever set this and you are using it from
> > addr_object_size which is also used for other C family languages
> > (at least), I wonder if you can really test
> > 
> > +                       if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> > 
> > there.
> 
> You mean for C++ and also other C family languages (other than C), the above bit cannot be set?
> Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY, which is only set for C too. 
> So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set TYPE_INCLUDE_FLEXARRAY also in C++ FE?
> What’s your suggestion?
> 
> (I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be set in the same places).

I was wondering if the above test errors on the conservative side
correctly - it will now, for all but C, cut off some thing where it
didn't before?

> > 
> > Originally I was suggesting to set this flag in stor-layout.cc
> > which eventually all languages funnel their types through and
> > if there's language specific handling use a langhook (with the
> > default implementation preserving the status quo).
> 
> If we decide to set the bits in stor-layout.cc, where is the best place to do it? I checked the star-layout.cc code, looks like “layout_type” might be the place where we can set these bits for RECORD_TYPE, UNION_TYPE? 

Yes, it would be layout_type.

> > 
> > Some more comments below ...
> > 
> >> gcc/cp/ChangeLog:
> >> 
> >> 	PR tree-optimization/101832
> >> 	* module.cc (trees_out::core_bools): Stream out new bit
> >> 	type_include_flexarray.
> >> 	(trees_in::core_bools): Stream in new bit type_include_flexarray.
> >> 
> >> gcc/ChangeLog:
> >> 
> >> 	PR tree-optimization/101832
> >> 	* print-tree.cc (print_node): Print new bit type_include_flexarray.
> >> 	* tree-core.h (struct tree_type_common): New bit
> >> 	type_include_flexarray.
> >> 	* tree-object-size.cc (addr_object_size): Handle structure/union type
> >> 	when it has flexible size.
> >> 	* tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
> >> 	in new bit type_include_flexarray.
> >> 	* tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
> >> 	out new bit type_include_flexarray.
> >> 	* tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
> >> 	TYPE_INCLUDE_FLEXARRAY.
> >> 
> >> gcc/testsuite/ChangeLog:
> >> 
> >> 	PR tree-optimization/101832
> >> 	* gcc.dg/builtin-object-size-pr101832.c: New test.
> >> ---
> >> gcc/c/c-decl.cc                               |  12 ++
> >> gcc/cp/module.cc                              |   2 +
> >> gcc/print-tree.cc                             |   5 +
> >> .../gcc.dg/builtin-object-size-pr101832.c     | 134 ++++++++++++++++++
> >> gcc/tree-core.h                               |   4 +-
> >> gcc/tree-object-size.cc                       |  79 +++++++----
> >> gcc/tree-streamer-in.cc                       |   1 +
> >> gcc/tree-streamer-out.cc                      |   1 +
> >> gcc/tree.h                                    |   6 +
> >> 9 files changed, 215 insertions(+), 29 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >> 
> >> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> >> index 08078eadeb8..f589a2f5192 100644
> >> --- a/gcc/c/c-decl.cc
> >> +++ b/gcc/c/c-decl.cc
> >> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> >>       /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x.  */
> >>       DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
> >> 
> >> +      /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> >> +       * when x is an array.  */
> >> +      if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
> >> +	TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
> >> +      /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> >> +	 when x is the last field.  */
> >> +      else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
> >> +		|| TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
> >> +	       && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
> >> +	       && is_last_field)
> >> +	TYPE_INCLUDE_FLEXARRAY (t) = true;
> >> +
> >>       if (DECL_NAME (x)
> >> 	  || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> >> 	saw_named_field = true;
> >> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> >> index ac2fe66b080..c750361b704 100644
> >> --- a/gcc/cp/module.cc
> >> +++ b/gcc/cp/module.cc
> >> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
> >>       WB (t->type_common.lang_flag_5);
> >>       WB (t->type_common.lang_flag_6);
> >>       WB (t->type_common.typeless_storage);
> >> +      WB (t->type_common.type_include_flexarray);
> >>     }
> >> 
> >>   if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> >> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
> >>       RB (t->type_common.lang_flag_5);
> >>       RB (t->type_common.lang_flag_6);
> >>       RB (t->type_common.typeless_storage);
> >> +      RB (t->type_common.type_include_flexarray);
> >>     }
> >> 
> >>   if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> >> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
> >> index 1f3afcbbc86..efacdb7686f 100644
> >> --- a/gcc/print-tree.cc
> >> +++ b/gcc/print-tree.cc
> >> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
> >> 	  && TYPE_CXX_ODR_P (node))
> >> 	fputs (" cxx-odr-p", file);
> >> 
> >> +      if ((code == RECORD_TYPE
> >> +	   || code == UNION_TYPE)
> >> +	  && TYPE_INCLUDE_FLEXARRAY (node))
> >> +	fputs (" include-flexarray", file);
> >> +
> >>       /* The transparent-union flag is used for different things in
> >> 	 different nodes.  */
> >>       if ((code == UNION_TYPE || code == RECORD_TYPE)
> >> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >> new file mode 100644
> >> index 00000000000..60078e11634
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >> @@ -0,0 +1,134 @@
> >> +/* PR 101832: 
> >> +   GCC extension accepts the case when a struct with a C99 flexible array
> >> +   member is embedded into another struct (possibly recursively).
> >> +   __builtin_object_size will treat such struct as flexible size.
> >> +   However, when a structure with non-C99 flexible array member, i.e, trailing
> >> +   [0], [1], or [4], is embedded into anther struct, the stucture will not
> >> +   be treated as flexible size.  */ 
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O2" } */
> >> +
> >> +#include "builtin-object-size-common.h"
> >> +
> >> +#define expect(p, _v) do { \
> >> +  size_t v = _v; \
> >> +  if (p == v) \
> >> +    __builtin_printf ("ok:  %s == %zd\n", #p, p); \
> >> +  else {\
> >> +    __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
> >> +    FAIL (); \
> >> +  } \
> >> +} while (0);
> >> +
> >> +
> >> +struct A {
> >> +  int n;
> >> +  char data[];
> >> +};
> >> +
> >> +struct B {
> >> +  int m;
> >> +  struct A a;
> >> +};
> >> +
> >> +struct C {
> >> +  int q;
> >> +  struct B b;
> >> +};
> >> +
> >> +struct A0 {
> >> +  int n;
> >> +  char data[0];
> >> +};
> >> +
> >> +struct B0 {
> >> +  int m;
> >> +  struct A0 a;
> >> +};
> >> +
> >> +struct C0 {
> >> +  int q;
> >> +  struct B0 b;
> >> +};
> >> +
> >> +struct A1 {
> >> +  int n;
> >> +  char data[1];
> >> +};
> >> +
> >> +struct B1 {
> >> +  int m;
> >> +  struct A1 a;
> >> +};
> >> +
> >> +struct C1 {
> >> +  int q;
> >> +  struct B1 b;
> >> +};
> >> +
> >> +struct An {
> >> +  int n;
> >> +  char data[8];
> >> +};
> >> +
> >> +struct Bn {
> >> +  int m;
> >> +  struct An a;
> >> +};
> >> +
> >> +struct Cn {
> >> +  int q;
> >> +  struct Bn b;
> >> +};
> >> +
> >> +volatile void *magic1, *magic2;
> >> +
> >> +int main (int argc, char *argv[])
> >> +{
> >> +  struct B *outer;
> >> +  struct C *outest;
> >> +
> >> +  /* Make sure optimization can't find some other object size. */
> >> +  outer = (void *)magic1;
> >> +  outest = (void *)magic2;
> >> +
> >> +  expect (__builtin_object_size (&outer->a, 1), -1);
> >> +  expect (__builtin_object_size (&outest->b, 1), -1);
> >> +  expect (__builtin_object_size (&outest->b.a, 1), -1);
> >> +
> >> +  struct B0 *outer0;
> >> +  struct C0 *outest0;
> >> +
> >> +  /* Make sure optimization can't find some other object size. */
> >> +  outer0 = (void *)magic1;
> >> +  outest0 = (void *)magic2;
> >> +
> >> +  expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
> >> +  expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
> >> +  expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
> >> +
> >> +  struct B1 *outer1;
> >> +  struct C1 *outest1;
> >> +
> >> +  /* Make sure optimization can't find some other object size. */
> >> +  outer1 = (void *)magic1;
> >> +  outest1 = (void *)magic2;
> >> +
> >> +  expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
> >> +  expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
> >> +  expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
> >> +
> >> +  struct Bn *outern;
> >> +  struct Cn *outestn;
> >> +
> >> +  /* Make sure optimization can't find some other object size. */
> >> +  outern = (void *)magic1;
> >> +  outestn = (void *)magic2;
> >> +
> >> +  expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
> >> +  expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
> >> +  expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
> >> +
> >> +  DONE ();
> >> +  return 0;
> >> +}
> >> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> >> index acd8deea34e..705d5702b9c 100644
> >> --- a/gcc/tree-core.h
> >> +++ b/gcc/tree-core.h
> >> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
> >>   unsigned empty_flag : 1;
> >>   unsigned indivisible_p : 1;
> >>  RECORD_OR_UNION_CHECK
> > 
> > it looks like the bit could be eventually shared with
> > no_named_args_stdarg_p (but its accessor doesn't use
> > FUNC_OR_METHOD_CHECK)
> You mean to let “type_include_flexarray” share the same bit with “no_named_args_stdarg_p” in order to save 1-bit space in IR?
> Then change the 
> 
> /* True if this is a stdarg function with no named arguments (C2x
>    (...) prototype, where arguments can be accessed with va_start and
>    va_arg), as opposed to an unprototyped function.  */
> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>   (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> 
> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>    at the last field recursively.  */
> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>   (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
> 
> To:
> 
> union {
>     unsigned no_named_args_stdarg_p : 1;
>     /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE.  */
>     unsigned int type_include_flexarray : 1;
>   } shared_bit;
>   unsigned spare : 15;
> 
> /* True if this is a stdarg function with no named arguments (C2x
>    (...) prototype, where arguments can be accessed with va_start and
>    va_arg), as opposed to an unprototyped function.  */
> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>   (FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
> 
> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>    at the last field recursively.  */
> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>   (RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)

No, we're just overloading bits by using the same name for different
purposes.  I don't think such a union would pack nicely.  We overload
by documenting the uses, in the above cases the uses are separated
by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE
and the accessor macros should be updated to use the appropriate
tree check macros covering that so we don't try to inspect the
"wrong bit".

> > 
> >>   alias_set_type alias_set;
> >>   tree pointer_to;
> >> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> >> index 9a936a91983..22b3c72ea6e 100644
> >> --- a/gcc/tree-object-size.cc
> >> +++ b/gcc/tree-object-size.cc
> >> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
> >> 		    v = NULL_TREE;
> >> 		    break;
> >> 		  case COMPONENT_REF:
> >> -		    if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> >> +		    /* When the ref is not to an array, a record or a union, it
> >> +		       will not have flexible size, compute the object size
> >> +		       directly.  */
> >> +		    if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> >> +			&& (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
> >> +			&& (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
> >> 		      {
> >> 			v = NULL_TREE;
> >> 			break;
> >> 		      }
> >> -		    is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
> >> -		    while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> -		      if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> -			  != UNION_TYPE
> >> -			  && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> -			  != QUAL_UNION_TYPE)
> >> -			break;
> >> -		      else
> >> -			v = TREE_OPERAND (v, 0);
> >> -		    if (TREE_CODE (v) == COMPONENT_REF
> >> -			&& TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> -			   == RECORD_TYPE)
> >> +		    if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
> >> +			|| TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
> >> +		    /* if the record or union does not include a flexible array
> >> +		       recursively, compute the object size directly.  */
> >> 		      {
> >> -			/* compute object size only if v is not a
> >> -			   flexible array member.  */
> >> -			if (!is_flexible_array_mem_ref)
> >> +			if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> >> 			  {
> >> 			    v = NULL_TREE;
> >> 			    break;
> >> 			  }
> >> -			v = TREE_OPERAND (v, 0);
> >> +			else
> >> +			  v = TREE_OPERAND (v, 0);
> >> 		      }
> >> -		    while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> -		      if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> -			  != UNION_TYPE
> >> -			  && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> -			  != QUAL_UNION_TYPE)
> >> -			break;
> >> -		      else
> >> -			v = TREE_OPERAND (v, 0);
> >> -		    if (v != pt_var)
> >> -		      v = NULL_TREE;
> >> 		    else
> >> -		      v = pt_var;
> >> +		      {
> >> +			/* Now the ref is to an array type.  */
> >> +			is_flexible_array_mem_ref
> >> +			  = array_ref_flexible_size_p (v);
> >> +			while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> +			if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> +			      != UNION_TYPE
> >> +			    && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> +				 != QUAL_UNION_TYPE)
> >> +			  break;
> >> +			else
> >> +			  v = TREE_OPERAND (v, 0);
> >> +			if (TREE_CODE (v) == COMPONENT_REF
> >> +			    && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> +				 == RECORD_TYPE)
> >> +			  {
> >> +			    /* compute object size only if v is not a
> >> +			       flexible array member.  */
> >> +			    if (!is_flexible_array_mem_ref)
> >> +			      {
> >> +				v = NULL_TREE;
> >> +				break;
> >> +			      }
> >> +			    v = TREE_OPERAND (v, 0);
> >> +			  }
> >> +			while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> +			  if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> +				!= UNION_TYPE
> >> +			      && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> +				   != QUAL_UNION_TYPE)
> >> +			    break;
> >> +			  else
> >> +			    v = TREE_OPERAND (v, 0);
> >> +			if (v != pt_var)
> >> +			  v = NULL_TREE;
> >> +			else
> >> +			  v = pt_var;
> >> +		      }
> >> 		    break;
> >> 		  default:
> >> 		    v = pt_var;
> >> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
> >> index d4dc30f048f..c19ede0631d 100644
> >> --- a/gcc/tree-streamer-in.cc
> >> +++ b/gcc/tree-streamer-in.cc
> >> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> >>       TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>       TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>       TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> >> +      TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>     }
> >>   else if (TREE_CODE (expr) == ARRAY_TYPE)
> >>     TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
> >> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
> >> index d107229da5c..73e4b4e547c 100644
> >> --- a/gcc/tree-streamer-out.cc
> >> +++ b/gcc/tree-streamer-out.cc
> >> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> >>       bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
> >> 			 ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
> >> 			 : TYPE_CXX_ODR_P (expr), 1);
> >> +      bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
> >>     }
> >>   else if (TREE_CODE (expr) == ARRAY_TYPE)
> >>     bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
> >> diff --git a/gcc/tree.h b/gcc/tree.h
> >> index 92ac0e6a214..ab1cdc3dc85 100644
> >> --- a/gcc/tree.h
> >> +++ b/gcc/tree.h
> >> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
> >> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> >>   (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> >> 
> >> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> >> +   at the last field recursively.  */
> >> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
> >> +  (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
> > 
> > Please use RECORD_OR_UNION_CHECK.  
> Okay. 
> > The comment "at the last field"
> > doesn't seem to match implementation? (at least not obviously)
> The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the last field. (And recursively). 
> > Given this is a generic header expanding on what a "flexible array member"
> > is to the middle-end here would be good.  Especially the guarantees
> > made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
> > vs. DECL_FLEXARRAY).
> 
> The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex array by default.
> It’s only set to TRUE when the struct/union includes a flex array at the last field (and recursively). So, I think it should be correct. 
> Let me know if I missed anything here.

See above - I was looking at the use (but I'm not familiar with that 
code), and wondered how the change affects uses from other frontends.

Richard.