public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Biener <rguenther@suse.de>
To: Qing Zhao <qing.zhao@oracle.com>
Cc: gcc-patches Paul A Clarke via <gcc-patches@gcc.gnu.org>,
	 jakub Jelinek <jakub@redhat.com>,
	martin Sebor <msebor@gmail.com>,
	 kees Cook <keescook@chromium.org>,
	 "joseph@codesourcery.com" <joseph@codesourcery.com>
Subject: Re: [GCC13][Patch][V2][1/2]Add a new option -fstrict-flex-array[=n] and attribute strict_flex_array(n) and use it in PR101836
Date: Tue, 2 Aug 2022 07:03:53 +0000 (UTC)	[thread overview]
Message-ID: <nycvar.YFH.7.77.849.2208020649420.4208@jbgna.fhfr.qr> (raw)
In-Reply-To: <567D133A-E24A-4107-A843-95D2ED07ACF2@oracle.com>

On Mon, 1 Aug 2022, Qing Zhao wrote:

> 
> 
> > On Aug 1, 2022, at 3:38 AM, Richard Biener <rguenther@suse.de> wrote:
> > 
> > On Fri, 29 Jul 2022, Qing Zhao wrote:
> > 
> >> Hi, Richard,
> >> 
> >> Thanks a lot for your comments and suggestions. (And sorry for my late reply).
> >> 
> >>> On Jul 28, 2022, at 3:26 AM, Richard Biener <rguenther@suse.de> wrote:
> >>> 
> >>> On Tue, 19 Jul 2022, Qing Zhao wrote:
> >>> 
> >>>> From 3854004802b8e2f132ebf218fc35a632f5e80c6a Mon Sep 17 00:00:00 2001
> >>>> From: Qing Zhao <qing.zhao@oracle.com>
> >>>> Date: Mon, 18 Jul 2022 17:04:12 +0000
> >>>> Subject: [PATCH 1/2] Add a new option -fstrict-flex-array[=n] and new
> >>>> attribute strict_flex_array
> >>>> 
> >>>> Add the following new option -fstrict-flex-array[=n] and a corresponding
> >>>> attribute strict_flex_array to GCC:
> >>>> 
> >>>> '-fstrict-flex-array'
> >>>>   Treat the trailing array of a structure as a flexible array member
> >>>>   in a stricter way.  The positive form is equivalent to
> >>>>   '-fstrict-flex-array=3', which is the strictest.  A trailing array
> >>>>   is treated as a flexible array member only when it is declared as a
> >>>>   flexible array member per C99 standard onwards.  The negative form
> >>>>   is equivalent to '-fstrict-flex-array=0', which is the least
> >>>>   strict.  All trailing arrays of structures are treated as flexible
> >>>>   array members.
> >>>> 
> >>>> '-fstrict-flex-array=LEVEL'
> >>>>   Treat the trailing array of a structure as a flexible array member
> >>>>   in a stricter way.  The value of LEVEL controls the level of
> >>>>   strictness.
> >>>> 
> >>>>   The possible values of LEVEL are the same as for the
> >>>>   'strict_flex_array' attribute (*note Variable Attributes::).
> >>>> 
> >>>>   You can control this behavior for a specific trailing array field
> >>>>   of a structure by using the variable attribute 'strict_flex_array'
> >>>>   attribute (*note Variable Attributes::).
> >>>> 
> >>>> 'strict_flex_array (LEVEL)'
> >>>>   The 'strict_flex_array' attribute should be attached to the
> >>>>   trailing array field of a structure.  It specifies the level of
> >>>>   strictness of treating the trailing array field of a structure as a
> >>>>   flexible array member.  LEVEL must be an integer betwen 0 to 3.
> >>>> 
> >>>>   LEVEL=0 is the least strict level, all trailing arrays of
> >>>>   structures are treated as flexible array members.  LEVEL=3 is the
> >>>>   strictest level, only when the trailing array is declared as a
> >>>>   flexible array member per C99 standard onwards ([]), it is treated
> >>>>   as a flexible array member.
> >>>> 
> >>>>   There are two more levels in between 0 and 3, which are provided to
> >>>>   support older codes that use GCC zero-length array extension ([0])
> >>>>   or one-size array as flexible array member ([1]): When LEVEL is 1,
> >>>>   the trailing array is treated as a flexible array member when it is
> >>>>   declared as either [], [0], or [1]; When LEVEL is 2, the trailing
> >>>>   array is treated as a flexible array member when it is declared as
> >>>>   either [], or [0].
> >>>> 
> >>>>   This attribute can be used with or without '-fstrict-flex-array'.
> >>>>   When both the attribute and the option present at the same time,
> >>>>   the level of the strictness for the specific trailing array field
> >>>>   is determined by the attribute.
> >>>> 
> >>>> gcc/c-family/ChangeLog:
> >>>> 
> >>>> 	* c-attribs.cc (handle_strict_flex_array_attribute): New function.
> >>>> 	(c_common_attribute_table): New item for strict_flex_array.
> >>>> 	* c.opt (fstrict-flex-array): New option.
> >>>> 	(fstrict-flex-array=): New option.
> >>>> 
> >>>> gcc/c/ChangeLog:
> >>>> 
> >>>> 	* c-decl.cc (add_flexible_array_elts_to_size): Call new utility
> >>>> 	routine flexible_array_member_p.
> >>>> 	(is_flexible_array_member_p): New function.
> >>>> 	(finish_struct): Set the new DECL_NOT_FLEXARRAY flag.
> >>>> 
> >>>> gcc/ChangeLog:
> >>>> 
> >>>> 	* doc/extend.texi: Document strict_flex_array attribute.
> >>>> 	* doc/invoke.texi: Document -fstrict-flex-array[=n] option.
> >>>> 	* tree-core.h (struct tree_decl_common): New bit field
> >>>> 	decl_not_flexarray.
> >>>> 	* tree.cc (component_ref_size): Reorg by using new utility functions.
> >>>> 	(flexible_array_member_p): New function.
> >>>> 	(zero_length_array_p): Likewise.
> >>>> 	(one_element_array_p): Likewise.
> >>>> 	(flexible_array_type_p): Likewise.
> >>>> 	* tree.h (DECL_NOT_FLEXARRAY): New flag.
> >>>> 	(zero_length_array_p): New function prototype.
> >>>> 	(one_element_array_p): Likewise.
> >>>> 	(flexible_array_member_p): Likewise.
> >>>> 
> >>>> gcc/testsuite/ChangeLog:
> >>>> 
> >>>> 	* gcc.dg/strict-flex-array-1.c: New test.
> >>>> ---
> >>>> gcc/c-family/c-attribs.cc                  |  47 ++++++++
> >>>> gcc/c-family/c.opt                         |   7 ++
> >>>> gcc/c/c-decl.cc                            |  91 +++++++++++++--
> >>>> gcc/doc/extend.texi                        |  25 ++++
> >>>> gcc/doc/invoke.texi                        |  27 ++++-
> >>>> gcc/testsuite/gcc.dg/strict-flex-array-1.c |  31 +++++
> >>>> gcc/tree-core.h                            |   5 +-
> >>>> gcc/tree.cc                                | 130 ++++++++++++++-------
> >>>> gcc/tree.h                                 |  16 ++-
> >>>> 9 files changed, 322 insertions(+), 57 deletions(-)
> >>>> create mode 100644 gcc/testsuite/gcc.dg/strict-flex-array-1.c
> >>>> 
> >>>> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> >>>> index c8d96723f4c..10d16532f0d 100644
> >>>> --- a/gcc/c-family/c-attribs.cc
> >>>> +++ b/gcc/c-family/c-attribs.cc
> >>>> @@ -101,6 +101,8 @@ static tree handle_special_var_sec_attribute (tree *, tree, tree, int, bool *);
> >>>> static tree handle_aligned_attribute (tree *, tree, tree, int, bool *);
> >>>> static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree,
> >>>> 						  int, bool *);
> >>>> +static tree handle_strict_flex_array_attribute (tree *, tree, tree,
> >>>> +						int, bool *);
> >>>> static tree handle_weak_attribute (tree *, tree, tree, int, bool *) ;
> >>>> static tree handle_noplt_attribute (tree *, tree, tree, int, bool *) ;
> >>>> static tree handle_alias_ifunc_attribute (bool, tree *, tree, tree, bool *);
> >>>> @@ -367,6 +369,8 @@ const struct attribute_spec c_common_attribute_table[] =
> >>>> 	                      attr_aligned_exclusions },
> >>>> { "warn_if_not_aligned",    0, 1, false, false, false, false,
> >>>> 			      handle_warn_if_not_aligned_attribute, NULL },
> >>>> +  { "strict_flex_array",      1, 1, false, false, false, false,
> >>>> +			      handle_strict_flex_array_attribute, NULL },
> >>>> { "weak",                   0, 0, true,  false, false, false,
> >>>> 			      handle_weak_attribute, NULL },
> >>>> { "noplt",                   0, 0, true,  false, false, false,
> >>>> @@ -2498,6 +2502,49 @@ handle_warn_if_not_aligned_attribute (tree *node, tree name,
> >>>> 					  no_add_attrs, true);
> >>>> }
> >>>> 
> >>>> +/* Handle a "strict_flex_array" attribute; arguments as in
> >>>> +   struct attribute_spec.handler.  */
> >>>> +
> >>>> +static tree
> >>>> +handle_strict_flex_array_attribute (tree *node, tree name,
> >>>> +				    tree args, int ARG_UNUSED (flags),
> >>>> +				    bool *no_add_attrs)
> >>>> +{
> >>>> +  tree decl = *node;
> >>>> +  tree argval = TREE_VALUE (args);
> >>>> +
> >>>> +  /* This attribute only applies to field decls of a structure.  */
> >>>> +  if (TREE_CODE (decl) != FIELD_DECL)
> >>>> +    {
> >>>> +      error_at (DECL_SOURCE_LOCATION (decl),
> >>>> +		"%qE attribute may not be specified for %q+D", name, decl);
> >>>> +      *no_add_attrs = true;
> >>>> +    }
> >>>> +  /* This attribute only applies to field with array type.  */
> >>>> +  else if (TREE_CODE (TREE_TYPE (decl)) != ARRAY_TYPE)
> >>>> +    {
> >>>> +      error_at (DECL_SOURCE_LOCATION (decl),
> >>>> +		"%qE attribute may not be specified for a non array field",
> >>>> +		name);
> >>>> +      *no_add_attrs = true;
> >>>> +    }
> >>>> +  else if (TREE_CODE (argval) != INTEGER_CST)
> >>>> +    {
> >>>> +      error_at (DECL_SOURCE_LOCATION (decl),
> >>>> +		"%qE attribute argument not an integer", name);
> >>>> +      *no_add_attrs = true;
> >>>> +    }
> >>>> +  else if (!tree_fits_uhwi_p (argval) || tree_to_uhwi (argval) > 3)
> >>>> +    {
> >>>> +      error_at (DECL_SOURCE_LOCATION (decl),
> >>>> +		"%qE attribute argument %qE is not an integer constant"
> >>>> +		" between 0 and 3", name, argval);
> >>>> +      *no_add_attrs = true;
> >>>> +    }
> >>>> +
> >>>> +  return NULL_TREE;
> >>>> +}
> >>>> +
> >>>> /* Handle a "weak" attribute; arguments as in
> >>>>  struct attribute_spec.handler.  */
> >>>> 
> >>>> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> >>>> index 44e1a60ce24..864cd8df1d3 100644
> >>>> --- a/gcc/c-family/c.opt
> >>>> +++ b/gcc/c-family/c.opt
> >>>> @@ -2060,6 +2060,13 @@ fsized-deallocation
> >>>> C++ ObjC++ Var(flag_sized_deallocation) Init(-1)
> >>>> Enable C++14 sized deallocation support.
> >>>> 
> >>>> +fstrict-flex-array
> >>>> +C C++ Common Alias(fstrict-flex-array=,3,0)
> >>>> +
> >>>> +fstrict-flex-array=
> >>>> +C C++ Common Joined RejectNegative UInteger Var(flag_strict_flex_array) Init(0) IntegerRange(0,3)
> >>>> +-fstrict-flex-array=<level>    Treat the trailing array of a structure as a flexible array in a stricter way.  The default is treating all trailing arrays of structures as flexible arrays.
> >>>> +
> >>>> fsquangle
> >>>> C++ ObjC++ WarnRemoved
> >>>> 
> >>>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> >>>> index ae8990c138f..14defae9584 100644
> >>>> --- a/gcc/c/c-decl.cc
> >>>> +++ b/gcc/c/c-decl.cc
> >>>> @@ -5013,10 +5013,7 @@ add_flexible_array_elts_to_size (tree decl, tree init)
> >>>> 
> >>>> elt = CONSTRUCTOR_ELTS (init)->last ().value;
> >>>> type = TREE_TYPE (elt);
> >>>> -  if (TREE_CODE (type) == ARRAY_TYPE
> >>>> -      && TYPE_SIZE (type) == NULL_TREE
> >>>> -      && TYPE_DOMAIN (type) != NULL_TREE
> >>>> -      && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE)
> >>>> +  if (flexible_array_member_p (type))
> >>>>   {
> >>>>     complete_array_type (&type, elt, false);
> >>>>     DECL_SIZE (decl)
> >>>> @@ -8720,6 +8717,80 @@ finish_incomplete_vars (tree incomplete_vars, bool toplevel)
> >>>>   }
> >>>> }
> >>>> 
> >>>> +/* Determine whether the FIELD_DECL X is a flexible array member according to
> >>>> +   the following info:
> >>>> +  A. whether the FIELD_DECL X is the last field of the DECL_CONTEXT;
> >>>> +  B. whether the FIELD_DECL is an array that is declared as "[]", "[0]",
> >>>> +     or "[1]";
> >>>> +  C. flag_strict_flex_array;
> >>>> +  D. the attribute strict_flex_array that is attached to the field
> >>>> +     if presenting.
> >>>> +  Return TRUE when it's a flexible array member, FALSE otherwise.  */
> >>>> +
> >>>> +static bool
> >>>> +is_flexible_array_member_p (bool is_last_field,
> >>>> +			    tree x)
> >>>> +{
> >>>> +  /* if not the last field, return false.  */
> >>>> +  if (!is_last_field)
> >>>> +    return false;
> >>>> +
> >>>> +  /* if not an array field, return false.  */
> >>>> +  if (TREE_CODE (TREE_TYPE (x)) != ARRAY_TYPE)
> >>>> +    return false;
> >>>> +
> >>>> +  bool is_zero_length_array = zero_length_array_p (TREE_TYPE (x));
> >>>> +  bool is_one_element_array = one_element_array_p (TREE_TYPE (x));
> >>>> +  bool is_flexible_array = flexible_array_member_p (TREE_TYPE (x));
> >>>> +
> >>>> +  unsigned int strict_flex_array_level = flag_strict_flex_array;
> >>>> +
> >>>> +  tree attr_strict_flex_array = lookup_attribute ("strict_flex_array",
> >>>> +						  DECL_ATTRIBUTES (x));
> >>>> +  /* if there is a strict_flex_array attribute attached to the field,
> >>>> +     override the flag_strict_flex_array.  */
> >>>> +  if (attr_strict_flex_array)
> >>>> +    {
> >>>> +      /* get the value of the level first from the attribute.  */
> >>>> +      unsigned HOST_WIDE_INT attr_strict_flex_array_level = 0;
> >>>> +      gcc_assert (TREE_VALUE (attr_strict_flex_array) != NULL_TREE);
> >>>> +      attr_strict_flex_array = TREE_VALUE (attr_strict_flex_array);
> >>>> +      gcc_assert (TREE_VALUE (attr_strict_flex_array) != NULL_TREE);
> >>>> +      attr_strict_flex_array = TREE_VALUE (attr_strict_flex_array);
> >>>> +      gcc_assert (tree_fits_uhwi_p (attr_strict_flex_array));
> >>>> +      attr_strict_flex_array_level = tree_to_uhwi (attr_strict_flex_array);
> >>>> +
> >>>> +      /* the attribute has higher priority than flag_struct_flex_array.  */
> >>>> +      strict_flex_array_level = attr_strict_flex_array_level;
> >>>> +    }
> >>>> +
> >>>> +  switch (strict_flex_array_level)
> >>>> +    {
> >>>> +      case 0:
> >>>> +	/* default, all trailing arrays are flexiable array members.  */
> >>>> +	return true;
> >>>> +      case 1:
> >>>> +	/* Level 1: all "[1]", "[0]", and "[]" are flexiable array members.  */
> >>>> +	if (is_one_element_array)
> >>>> +	  return true;
> >>>> +	/* FALLTHROUGH.  */
> >>>> +      case 2:
> >>>> +	/* Level 2: all "[0]", and "[]" are flexiable array members.  */
> >>>> +	if (is_zero_length_array)
> >>>> +	  return true;
> >>>> +	/* FALLTHROUGH.  */
> >>>> +      case 3:
> >>>> +	/* Level 3: Only "[]" are flexible array members.  */
> >>>> +	if (is_flexible_array)
> >>>> +	  return true;
> >>>> +	break;
> >>>> +      default:
> >>>> +	gcc_unreachable ();
> >>>> +    }
> >>>> +  return false;
> >>>> +}
> >>>> +
> >>>> +
> >>>> /* Fill in the fields of a RECORD_TYPE or UNION_TYPE node, T.
> >>>>  LOC is the location of the RECORD_TYPE or UNION_TYPE's definition.
> >>>>  FIELDLIST is a chain of FIELD_DECL nodes for the fields.
> >>>> @@ -8781,6 +8852,8 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> >>>> bool saw_named_field = false;
> >>>> for (x = fieldlist; x; x = DECL_CHAIN (x))
> >>>>   {
> >>>> +      bool is_last_field = (DECL_CHAIN (x) == NULL_TREE);
> >>>> +
> >>>>     if (TREE_TYPE (x) == error_mark_node)
> >>>> 	continue;
> >>>> 
> >>>> @@ -8819,10 +8892,7 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> >>>> 	DECL_PACKED (x) = 1;
> >>>> 
> >>>>     /* Detect flexible array member in an invalid context.  */
> >>>> -      if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE
> >>>> -	  && TYPE_SIZE (TREE_TYPE (x)) == NULL_TREE
> >>>> -	  && TYPE_DOMAIN (TREE_TYPE (x)) != NULL_TREE
> >>>> -	  && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (x))) == NULL_TREE)
> >>>> +      if (flexible_array_member_p (TREE_TYPE (x)))
> >>>> 	{
> >>>> 	  if (TREE_CODE (t) == UNION_TYPE)
> >>>> 	    {
> >>>> @@ -8830,7 +8900,7 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> >>>> 			"flexible array member in union");
> >>>> 	      TREE_TYPE (x) = error_mark_node;
> >>>> 	    }
> >>>> -	  else if (DECL_CHAIN (x) != NULL_TREE)
> >>>> +	  else if (!is_last_field)
> >>>> 	    {
> >>>> 	      error_at (DECL_SOURCE_LOCATION (x),
> >>>> 			"flexible array member not at end of struct");
> >>>> @@ -8850,6 +8920,9 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> >>>> 	pedwarn (DECL_SOURCE_LOCATION (x), OPT_Wpedantic,
> >>>> 		 "invalid use of structure with flexible array member");
> >>>> 
> >>>> +      /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x.  */
> >>>> +      DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
> >>>> +
> >>>>     if (DECL_NAME (x)
> >>>> 	  || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> >>>> 	saw_named_field = true;
> >>>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> >>>> index dfbe33ac652..7451410a011 100644
> >>>> --- a/gcc/doc/extend.texi
> >>>> +++ b/gcc/doc/extend.texi
> >>>> @@ -7436,6 +7436,31 @@ This warning can be disabled by @option{-Wno-if-not-aligned}.
> >>>> The @code{warn_if_not_aligned} attribute can also be used for types
> >>>> (@pxref{Common Type Attributes}.)
> >>>> 
> >>>> +@cindex @code{strict_flex_array} variable attribute
> >>>> +@item strict_flex_array (@var{level})
> >>>> +The @code{strict_flex_array} attribute should be attached to the trailing
> >>>> +array field of a structure.  It specifies the level of strictness of
> >>>> +treating the trailing array field of a structure as a flexible array
> >>>> +member. @var{level} must be an integer betwen 0 to 3.
> >>>> +
> >>>> +@var{level}=0 is the least strict level, all trailing arrays of structures
> >>>> +are treated as flexible array members. @var{level}=3 is the strictest level,
> >>>> +only when the trailing array is declared as a flexible array member per C99
> >>>> +standard onwards ([]), it is treated as a flexible array member.
> >>> 
> >>> How is level 3 (thus -fstrict-flex-array) interpreted when you specify 
> >>> -std=c89?  How for -std=gnu89?
> >> 
> >> 1. what’s the major difference between -std=c89 and -std=gnu89 on flexible array? (Checked online, cannot find a concrete answer on this).
> >> 	** my understanding is:   -std=c89 will not support any flexible array (neither [], [0], [1]), but -std=gnu89 will support [0] and [1], but not [].
> >>         Is this correct?
> > 
> > Yes.
> > 
> >> If my answer to the first question is correct, then:
> >> 
> >> 2. When -fstrict-flex-array=n and -std=c89 present at the same time, which one has the higher priority? 
> >>     	** I think that -std=c89 should be honored over -fstrict-flex-array, therefore we should disable -fstrict-flex-array=n when n > 0  and issue warnings to the user.
> >> 
> >> 
> >> 3. how about -fstrict-flex-array=n and -std=gnu89 present at the same time? 
> >>     	** When -std=gnu89 present, [] is not supported. So, we need to issue an warning to disable -fstrict-flex-array=3; but level 1 and level 2 is Okay.
> >> 
> >> We also need to document the above.
> >> 
> >> Let me know if you have any more comment and suggestions here.
> > 
> > In my opinion -fstrict-flex-array only makes sense with C99 and above.  
> > The extra sub-levels allowing [0] or [1] are of questionable benefit.
> The multiple-levels control for -fstrict-flex-arrays was an agreement on the discussion of PR101836:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
> 
> Please see comment#17 till #25 for the details.
> 
> A summary of the discussion is:  (Please comment on the following paragraph if you see any issue with it)
> 
> =====
> Flexible Array Member (FAM) is a feature introduced in the C99 standard of the C language in May 2000.  It's used as the last element of a structure which is really a header for a variable-length object.
> This feature was officially standardized in C99. However, compilers (GCC, Micorsoft compiler, etc.) supported this feature well before C99, with the following variants:
>   A. trailing zero-length array of a structure was treated as FAM.  
>      Before C99, a valid GCC extension, zero-length array was used to support flexible array member.  See more details at:  https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html.
>   B. trailing one-element array of a structure was treated as FAM.
>      In the absence of the zero-length array extension, in ISO C90, one-element arrays at the end of structures were used to represent FAMs as a convertion.
>   C. any trailing array of a structure was treated as FAM.
>      Even earlier time, in the absence of both zero-length array GCC extension and one-element array convention, all trailing arrays of structures could be used as a FAM.     
> As a result, older codes (before C99) might include any of the above variants (A, B, and C) to represent FAMs. 
> Another complication is C++, standard C++ doesn't allow either C99 flexible array members or zero-length array. A C++ programmer has to use the GCC zero-length array extension or one-element array convention for the purpose of FAM.
> =====
> 
> As mentioned by Jakub in comment #25: 
> 
> "differentiating between only allowing [] as flex, or [] and [0],
> or [], [0] and [1], or any trailing array is useful."

OK.

> > 
> > I think that with -fstrict-flex-array we're missing a -Wstrict-flex-array
> > that would diagnose accesses that are no longer treated as flex array
> > access but would be without -fstrict-flex-array?  The goal of
> > -fstrict-flex-array + -Wstrict-flex-array is to make code standard
> > conformant, no?  Not to enable extra optimization for [1] arrays when
> > we know [0] is used as flexarray?
> 
> Yes, I agree, and this will make the feature more complete and more
>  useful to help users to get rid of the non-standard-comforming styles. 
> 
> Please see comments #23, #35
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836#c23
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836#c35
> 
> A summary is:
> 
> ====
> I think that -Wstrict-flex-arrays will need to be cooperated with -fstrict-flex-arrays=N, i.e, only when -fstrict-flex-arrays=N is presenting, -Wstrict-flex-arrays will be valid and report the warnings when see a [0], or [1], or any trailing array based on N:
> 
> when -fstrict-flex-arrays
> =0, -Wstrict-flex-arrays will NOT issue any warnings;
> =1, -Wstrict-flex-arrays will issue warnings when an array ref's index is larger than the upper bounds of a trailing array that is a regular trailing array;
> =2, -Wstrict-flex-arrays will issue warnings when an array ref's index is larger than the upper bounds of a trailing array that is a regular trailing array or [1];
> =3, -Wstrict-flex-arrays will issue warnings when an array ref's index is larger than the upper bounds of a trailing array that is a regular trailing array or [1], or [0].
> 
> let me know if you have any comment and suggestion on this.
> 
> ====

Ah, I see - good that this has been discussed.  -Wstrict-flex-array
shouldn't be a requirement for -fstrict-flex-array to go in, but it
would be nice to followup with an implementation for the above
(it might be tricky to find a good place for it, but eventually it
sounds like -Warray-bounds diagnosing places could eventually be
adjusted)

> 
> > 
> >>> 
> >>>> +
> >>>> +There are two more levels in between 0 and 3, which are provided to support
> >>>> +older codes that use GCC zero-length array extension ([0]) or one-size array
> >>>> +as flexible array member ([1]):
> >>>> +When @var{level} is 1, the trailing array is treated as a flexible array member
> >>>> +when it is declared as either "[]", "[0]", or "[1]";
> >>>> +When @var{level} is 2, the trailing array is treated as a flexible array member
> >>>> +when it is declared as either "[]", or "[0]".
> >>> 
> >>> Given the above does adding level 2 make sense given that [0] is a GNU
> >>> extension?
> >> I think Kees already answered this question.
> >>> 
> >>>> +This attribute can be used with or without @option{-fstrict-flex-array}. When
> >>>> +both the attribute and the option present at the same time, the level of the
> >>>> +strictness for the specific trailing array field is determined by the attribute.
> >>>> +
> >>>> +
> >>>> @item alloc_size (@var{position})
> >>>> @itemx alloc_size (@var{position-1}, @var{position-2})
> >>>> @cindex @code{alloc_size} variable attribute
> >>>> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> >>>> index 94fe57aa4e2..9befe601817 100644
> >>>> --- a/gcc/doc/invoke.texi
> >>>> +++ b/gcc/doc/invoke.texi
> >>>> @@ -207,7 +207,8 @@ in the following sections.
> >>>> -fopenmp  -fopenmp-simd @gol
> >>>> -fpermitted-flt-eval-methods=@var{standard} @gol
> >>>> -fplan9-extensions  -fsigned-bitfields  -funsigned-bitfields @gol
> >>>> --fsigned-char  -funsigned-char  -fsso-struct=@var{endianness}}
> >>>> +-fsigned-char  -funsigned-char -fstrict-flex-array[=@var{n}] @gol
> >>>> +-fsso-struct=@var{endianness}}
> >>>> 
> >>>> @item C++ Language Options
> >>>> @xref{C++ Dialect Options,,Options Controlling C++ Dialect}.
> >>>> @@ -2825,6 +2826,30 @@ The type @code{char} is always a distinct type from each of
> >>>> @code{signed char} or @code{unsigned char}, even though its behavior
> >>>> is always just like one of those two.
> >>>> 
> >>>> +@item -fstrict-flex-array
> >>>> +@opindex fstrict-flex-array
> >>>> +@opindex fno-strict-flex-array
> >>>> +Treat the trailing array of a structure as a flexible array member in a
> >>>> +stricter way.
> >>>> +The positive form is equivalent to @option{-fstrict-flex-array=3}, which is the
> >>>> +strictest.  A trailing array is treated as a flexible array member only when it
> >>>> +is declared as a flexible array member per C99 standard onwards.
> >>>> +The negative form is equivalent to @option{-fstrict-flex-array=0}, which is the
> >>>> +least strict.  All trailing arrays of structures are treated as flexible array
> >>>> +members.
> >>>> +
> >>>> +@item -fstrict-flex-array=@var{level}
> >>>> +@opindex fstrict-flex-array=@var{level}
> >>>> +Treat the trailing array of a structure as a flexible array member in a
> >>>> +stricter way.  The value of @var{level} controls the level of strictness.
> >>>> +
> >>>> +The possible values of @var{level} are the same as for the
> >>>> +@code{strict_flex_array} attribute (@pxref{Variable Attributes}).
> >>>> +
> >>>> +You can control this behavior for a specific trailing array field of a
> >>>> +structure by using the variable attribute @code{strict_flex_array} attribute
> >>>> +(@pxref{Variable Attributes}).
> >>>> +
> >>>> @item -fsso-struct=@var{endianness}
> >>>> @opindex fsso-struct
> >>>> Set the default scalar storage order of structures and unions to the
> >>>> diff --git a/gcc/testsuite/gcc.dg/strict-flex-array-1.c b/gcc/testsuite/gcc.dg/strict-flex-array-1.c
> >>>> new file mode 100644
> >>>> index 00000000000..ec886c99b25
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.dg/strict-flex-array-1.c
> >>>> @@ -0,0 +1,31 @@
> >>>> +/* testing the correct usage of attribute strict_flex_array.  */   
> >>>> +/* { dg-do compile } */
> >>>> +/* { dg-options "-O2" } */
> >>>> +
> >>>> +
> >>>> +int x __attribute__ ((strict_flex_array (1))); /* { dg-error "'strict_flex_array' attribute may not be specified for 'x'" } */
> >>>> +
> >>>> +struct trailing {
> >>>> +    int a;
> >>>> +    int c __attribute ((strict_flex_array)); /* { dg-error "wrong number of arguments specified for 'strict_flex_array' attribute" } */
> >>>> +};
> >>>> +
> >>>> +struct trailing_1 {
> >>>> +    int a;
> >>>> +    int b;
> >>>> +    int c __attribute ((strict_flex_array (2))); /* { dg-error "'strict_flex_array' attribute may not be specified for a non array field" } */
> >>>> +};
> >>>> +
> >>>> +extern int d;
> >>>> +
> >>>> +struct trailing_array_2 {
> >>>> +    int a;
> >>>> +    int b;
> >>>> +    int c[1] __attribute ((strict_flex_array (d))); /* { dg-error "'strict_flex_array' attribute argument not an integer" } */
> >>>> +};
> >>>> +
> >>>> +struct trailing_array_3 {
> >>>> +    int a;
> >>>> +    int b;
> >>>> +    int c[0] __attribute ((strict_flex_array (5))); /* { dg-error "'strict_flex_array' attribute argument '5' is not an integer constant between 0 and 3" } */
> >>>> +};
> >>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> >>>> index ea9f281f1cc..458c6e6ceea 100644
> >>>> --- a/gcc/tree-core.h
> >>>> +++ b/gcc/tree-core.h
> >>>> @@ -1813,7 +1813,10 @@ struct GTY(()) tree_decl_common {
> >>>>    TYPE_WARN_IF_NOT_ALIGN.  */
> >>>> unsigned int warn_if_not_align : 6;
> >>>> 
> >>>> -  /* 14 bits unused.  */
> >>>> +  /* In FIELD_DECL, this is DECL_NOT_FLEXARRAY.  */
> >>>> +  unsigned int decl_not_flexarray : 1;
> >>>> +
> >>>> +  /* 13 bits unused.  */
> >>> 
> >>> I've not seen it so you are probably missing it - the bit has to be
> >>> streamed in tree-streamer-{in,out}.cc to be usable from LTO.
> >> 
> >> You mean add it to the routine “unpack_ts_decl_common_value_fields” of tree-streamer-in.cc
> >> And “pack_ts_decl_common_value_fields” of tree-streamer-out.cc?
> > 
> > Yes.
> > 
> >> 
> >>> C++ module streaming also needs to handle it.
> >> 
> >> Which file is for this C++ module streaming?
> > 
> > Not sure, maybe cp/module.cc
> 
> Just checked this file, looks like it’s “Experimental”:
> /* C++ modules.  Experimental!
> 
> Is it used right now? To whom I can consult with to get more detail on C++?

There's testsuite in g++.dg/modules/, Nathan Sidwell should know since
he implemented it.

> > 
> >>> 
> >>>> 
> >>>> /* UID for points-to sets, stable over copying from inlining.  */
> >>>> unsigned int pt_uid;
> >>>> diff --git a/gcc/tree.cc b/gcc/tree.cc
> >>>> index 84000dd8b69..02e274699fb 100644
> >>>> --- a/gcc/tree.cc
> >>>> +++ b/gcc/tree.cc
> >>>> @@ -12862,7 +12862,7 @@ get_initializer_for (tree init, tree decl)
> >>>> /* Determines the size of the member referenced by the COMPONENT_REF
> >>>>  REF, using its initializer expression if necessary in order to
> >>>>  determine the size of an initialized flexible array member.
> >>>> -   If non-null, set *ARK when REF refers to an interior zero-length
> >>>> +   If non-null, set *SAM when REF refers to an interior zero-length
> >>>>  array or a trailing one-element array.
> >>>>  Returns the size as sizetype (which might be zero for an object
> >>>>  with an uninitialized flexible array member) or null if the size
> >>>> @@ -12878,16 +12878,32 @@ component_ref_size (tree ref, special_array_member *sam /* = NULL */)
> >>>>   sam = &sambuf;
> >>>> *sam = special_array_member::none;
> >>>> 
> >>>> +  /* Whether this ref is an array at the end of a structure.  */
> >>>> +  bool trailing = array_at_struct_end_p (ref);
> >>>> +
> >>> 
> >>> actually array_at_struct_end_p returns whether ref possibly refers to
> >>> a trailing array.  In particular it may return false for arrays at
> >>> struct end with a known length as in a.b[i] for the reference to global 
> >>> 'a':
> >>> 
> >>> struct { int b[1]; } a;
> >> 
> >> Yes, I noticed this behavior during my recent debugging of this routine. I thought it’s a bug and planned to file another bug against it.
> >> Looks like that this is an expected behavior.
> >> 
> >> Then I really feel the name and the comments of the routine is very confusing…
> >> Shall we change the name of this routine to a more descriptive one? For example, “flexible_array_member_p”? 
> > 
> > Note the routine is about classifying a concrete reference, so a better
> > name could be flex_array_ref_p ().  The name change should be done
> > separately (if at all).
> 
> Okay. 
> 
> > 
> >>> 
> >>> so in the end it should be array_at_struct_end_p also honoring
> >>> DECL_NOT_FLEXARRAY.
> >> 
> >> Then it’s make more sense to check DECL_NOT_FLEXARRAY inside this utility routine?
> > 
> > Yes.
> > 
> >>> 
> >>>> /* The object/argument referenced by the COMPONENT_REF and its type.  */
> >>>> tree arg = TREE_OPERAND (ref, 0);
> >>>> tree argtype = TREE_TYPE (arg);
> >>>> -  /* The referenced member.  */
> >>>> -  tree member = TREE_OPERAND (ref, 1);
> >>>> 
> >>>> +  /* The referenced field member.  */
> >>>> +  tree member = TREE_OPERAND (ref, 1);
> >>>> +  tree memtype = TREE_TYPE (member);
> >>>> tree memsize = DECL_SIZE_UNIT (member);
> >>>> +
> >>>> +  bool is_zero_length_array_ref = zero_length_array_p (memtype);
> >>>> +  bool is_constant_length_array_ref = false;
> >>>> +  bool is_one_element_array_ref
> >>>> +	= one_element_array_p (memtype, &is_constant_length_array_ref);
> >>>> +
> >>>> +  /* Determine the type of the special array member.  */
> >>>> +  if (is_zero_length_array_ref)
> >>>> +    *sam = trailing ? special_array_member::trail_0
> >>>> +	   : special_array_member::int_0;
> >>>> +  else if (is_one_element_array_ref && trailing)
> >>>> +    *sam = special_array_member::trail_1;
> >>>> +
> >>>> if (memsize)
> >>>>   {
> >>>> -      tree memtype = TREE_TYPE (member);
> >>>>     if (TREE_CODE (memtype) != ARRAY_TYPE)
> >>>> 	/* DECL_SIZE may be less than TYPE_SIZE in C++ when referring
> >>>> 	   to the type of a class with a virtual base which doesn't
> >>>> @@ -12897,50 +12913,30 @@ component_ref_size (tree ref, special_array_member *sam /* = NULL */)
> >>>> 	return (tree_int_cst_equal (memsize, TYPE_SIZE_UNIT (memtype))
> >>>> 		? memsize : NULL_TREE);
> >>>> 
> >>>> -      bool trailing = array_at_struct_end_p (ref);
> >>>> -      bool zero_length = integer_zerop (memsize);
> >>>> -      if (!trailing && !zero_length)
> >>>> +      if (!trailing && !is_zero_length_array_ref)
> >>>> 	/* MEMBER is either an interior array or is an array with
> >>>> 	   more than one element.  */
> >>>> 	return memsize;
> >>>> 
> >>>> -      if (zero_length)
> >>>> -	{
> >>>> -	  if (trailing)
> >>>> -	    *sam = special_array_member::trail_0;
> >>>> -	  else
> >>>> -	    {
> >>>> -	      *sam = special_array_member::int_0;
> >>>> -	      memsize = NULL_TREE;
> >>>> -	    }
> >>>> -	}
> >>>> +      if (*sam != special_array_member::trail_1
> >>>> +	  && is_constant_length_array_ref)
> >>>> +	/* MEMBER is a constant length array which is not a one-element
> >>>> +	   trailing array.  */
> >>>> +	return memsize;
> >>>> 
> >>>> -      if (!zero_length)
> >>>> -	if (tree dom = TYPE_DOMAIN (memtype))
> >>>> -	  if (tree min = TYPE_MIN_VALUE (dom))
> >>>> -	    if (tree max = TYPE_MAX_VALUE (dom))
> >>>> -	      if (TREE_CODE (min) == INTEGER_CST
> >>>> -		  && TREE_CODE (max) == INTEGER_CST)
> >>>> -		{
> >>>> -		  offset_int minidx = wi::to_offset (min);
> >>>> -		  offset_int maxidx = wi::to_offset (max);
> >>>> -		  offset_int neltsm1 = maxidx - minidx;
> >>>> -		  if (neltsm1 > 0)
> >>>> -		    /* MEMBER is an array with more than one element.  */
> >>>> -		    return memsize;
> >>>> -
> >>>> -		  if (neltsm1 == 0)
> >>>> -		    *sam = special_array_member::trail_1;
> >>>> -		}
> >>>> +      if (*sam == special_array_member::int_0)
> >>>> +	memsize = NULL_TREE;
> >>>> 
> >>>> -      /* For a reference to a zero- or one-element array member of a union
> >>>> -	 use the size of the union instead of the size of the member.  */
> >>>> +      /* For a reference to a flexible array member, an interior zero length
> >>>> +	 array, or an array with variable length of a union, use the size of
> >>>> +	 the union instead of the size of the member.  */
> >>>>     if (TREE_CODE (argtype) == UNION_TYPE)
> >>>> 	memsize = TYPE_SIZE_UNIT (argtype);
> >>>>   }
> >>>> 
> >>>> -  /* MEMBER is either a bona fide flexible array member, or a zero-length
> >>>> -     array member, or an array of length one treated as such.  */
> >>>> +  /* MEMBER now is a flexible array member, an interior zero length array, or
> >>>> +     an array with variable length.  We need to decide its size from its
> >>>> +     initializer.  */
> >>>> 
> >>>> /* If the reference is to a declared object and the member a true
> >>>>    flexible array, try to determine its size from its initializer.  */
> >>>> @@ -14351,6 +14347,59 @@ default_is_empty_record (const_tree type)
> >>>> return is_empty_type (TYPE_MAIN_VARIANT (type));
> >>>> }
> >>>> 
> >>>> +/* Determine whether TYPE is a ISO flexible array memeber type "[]".  */
> >>> 
> >>> ISO C99
> >> 
> >> Okay, will add this.
> >>> 
> >>>> +bool
> >>>> +flexible_array_member_p (const_tree type)
> >>> 
> >>> since you pass in a type a better name would be
> >>> flexible_array_type_p?
> >> Okay, how about “flexible_array_member_type_p”? (Since “flexible array member” is a complete concept).
> > 
> > works for me
> > 
> >>> 
> >>>> +{
> >>>> +  if (TREE_CODE (type) == ARRAY_TYPE
> >>>> +      && TYPE_SIZE (type) == NULL_TREE
> >>>> +      && TYPE_DOMAIN (type) != NULL_TREE
> >>> 
> >>> why require a specified TYPE_DOMAIN?
> >> 
> >> There are multiple places in the current GCC used the following sequence:
> >> 
> >> -      if (TREE_CODE (type)) == ARRAY_TYPE
> >> -         && TYPE_SIZE (type) == NULL_TREE
> >> -         && TYPE_DOMAIN (type) != NULL_TREE
> >> -         && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE)
> >> 
> >> To check whether the type is a flexible_array_member type.  
> >> (For example, the routine “add_flexible_array_elts_to_size” in c/c-decl.cc
> >> 			the routine “finish_struct” in c/c-decl.cc
> >> 			the routine “flexible_array_type_p” in tree.cc)
> >> 
> >> That’s the reason I come up with this common routine to replace all these sequences.
> >> 
> >> 
> >>>> +      && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE)
> >>> 
> >>> and why a NULL TYPE_MAX_VALUE?  Isn't an unknown TYPE_SIZE enough?
> >> 
> >> Does the current FE generate such IR for a []?  An array type, without TYPE_SIZE, with a TYPE_DOMAIN, but the MAX_VALUE of the TYPE_DOMAIN is NULL?
> >> 
> >>> 
> >>> That said, I'm not sure providing this abstraction is a good idea
> >>> given the use I see is in frontend code.
> >> 
> >> This one is also used in middle-end, for example “flexible_array_type_p” in tree.cc.
> > 
> > That was originally in c-decl.c, seems some target wanted to use it.  The
> > issue with this is that it tests the C frontend way of declaring a flex
> > array (and it is used by the frontend), but there are many other possible
> > representations the middle-end should classify as flex array.
> > 
> > The Objective C frontend also has a function named this way and it seems
> > to be a 1:1 copy :/
> > 
> > The nios2 use of the function doesn't make much sense to me - externals
> > should not end up in _any_ section!?
> 
> What’s you mean by “nios2” ?  Not quite understand this sentence. -:) could you explain a little bit?

nios2 is one of GCCs target architectures, it was a use in the nios2
backend this function was moved to generic code for but the actual use
doesn't really make sense to me ... (it's not your fault of course)

> > 
> > So I'd rather move this function back to where it came from, ideally
> > to c-family/, removing the copy in objective C ...
> 
> Okay, I will try  this in the next version.
> > 
> >>> 
> >>>> +    return true;
> >>>> +
> >>>> +  return false;
> >>>> +}
> >>>> +
> >>>> +/* Determine whether TYPE is a zero-length array type "[0]".  */
> >>>> +bool
> >>>> +zero_length_array_p (const_tree type)
> >>>> +{
> >>>> +  if (TREE_CODE (type) == ARRAY_TYPE)
> >>>> +    if (tree type_size = TYPE_SIZE_UNIT (type))
> >>>> +      if ((integer_zerop (type_size))
> >>>> +	   && TYPE_DOMAIN (type) != NULL_TREE
> >>>> +	   && TYPE_MAX_VALUE (TYPE_DOMAIN (type)) == NULL_TREE)
> >>> 
> >>> that again seems very C(?) frontend specific, please drop it.
> >> Currently, the middle-end utility routine “component_ref_size” need to check zero_length_array, one_element_array, etc, 
> >> So, this is not only a FE specific routine.
> > 
> > Sigh, it's nother strange function that was introduced just for 
> > diagnostics (so it's not "correct"), it shouldn't have ended up in
> > tree.cc IMHO.
> 
> The codes in GCC to handle flexible array are really messy, I hope that we can make it clean and consistent through this work. 
> Of course gradually…

As said, it's unfortunate to have many APIs doing similar things and
I'd rather avoid introducing new APIs (with to me, questionable
implementation or general usefulness) when possible.

> > 
> >> And I think that for making the -fstrict-flex-array work clear, we might want to emphasize  and distinguish the concepts of 
> >> 
> >> []          flexible_array_member_type_p
> >> [0]        zero_length_array_type_p
> >> [1]        one_element_array_type_p
> >> 
> >> Across GCC.
> > 
> > Why?
> 
> Because the whole confusion of flexible array handlings in our compiler started from the confusion of these concepts.
> During my study of this whole problem, I found that these concepts were really confusion, and after I was finally clear on
> These concepts, I started to know how to resolve the whole issue. 
> 
> In our GCC doc, https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> 
> We already have such concepts, but not very clear.  
> I think making these concepts clear is very important to resolve such messy situation in GCC.
>
> >  The only important thing to the middle-end is whether the
> > size of the array is known (aka array_at_struct_end_p).  This
> > routine classifies it with
> 
> But whether the size of the array is known depends on whether the array is a
> 
> [0],
> [1],
> []
> 
> +  -fstrict-flex-arrays=n

But only array_at_struct_end_p should care and for it we will have
DECL_NOT_FLEXARRAY.  And frontends may have different ideas how they
represent [0], [1] or [] (and in fact they do).  The rest of the
middle-end really should _not_ care.

> 
> > 
> >  /* 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)
> >      || ! TYPE_DOMAIN (atype)
> >      || ! TYPE_MAX_VALUE (TYPE_DOMAIN (atype)))
> >    return true;
> 
> This sequence is only for identifying []. Right?
> 
> The [0] and [1] is not handled by the above.

Yes.  But also int a[1:] would be identified this way, other frontends
like Ada or Fortran have means to more elaborately specify array
dimensions.

> > 
> > any flexible_array_member_type_p that covers a different subset of
> > flex-array types would be hugely confusing.
> 
> Don’t quite understand this sentence...

I mean if array_at_struct_end_p decides sth is a flex array reference
but flexible_array_member_type_p on the type decides it is not, that
would be confusing.  Thus if adding such new API then it should be
possible to use it from array_at_struct_end_p, removing then some
of the redundant checks there.

> > 
> >> 
> >>> 
> >>>> +	return true;
> >>>> +  return false;
> >>>> +}
> >>>> +
> >>>> +/* Determine whether TYPE is a one-element array type "[1]".
> >>>> +   Set IS_CONSTANT_LENGTH to true if the length is constant,
> >>>> +   otherwise, IS_CONSTANT_LENGTH is set to false.  */
> >>>> +bool
> >>>> +one_element_array_p (const_tree type, bool *is_constant_length /* = NULL */)
> >>>> +{
> >>>> +  if (is_constant_length)
> >>>> +    *is_constant_length = false;
> >>>> +
> >>>> +  if (TREE_CODE (type) == ARRAY_TYPE)
> >>>> +    if (tree dom = TYPE_DOMAIN (type))
> >>>> +      if (tree min = TYPE_MIN_VALUE (dom))
> >>>> +	if (tree max = TYPE_MAX_VALUE (dom))
> >>>> +	  if (TREE_CODE (min) == INTEGER_CST
> >>>> +	      && TREE_CODE (max) == INTEGER_CST)
> >>>> +	    {
> >>>> +	      offset_int minidx = wi::to_offset (min);
> >>>> +	      offset_int maxidx = wi::to_offset (max);
> >>>> +	      offset_int neltsm1 = maxidx - minidx;
> >>>> +	      if (is_constant_length)
> >>>> +		*is_constant_length = true;
> >>>> +	      if (neltsm1 == 0)
> >>>> +		return true;
> >>>> +	    }
> >>> 
> >>> I'd say likewise.  Maybe move them to c-family/c-common.{cc,h} instead
> >>> in a more specialized way for the single use you have?
> >> 
> >> It’s not single use, it’s also used in “component_ref_size” routine. 
> > 
> > Don't copy from a routine that's not designed to be "correct”.
> 
> Okay….
> 
> 
> >  In
> > fact the middle-end already has array_type_nelts, so one_element_array_p
> > can be written as integer_zerop (array_type_nelts (type)).  It also
> > has the comment
> > 
> >  /* TYPE_MAX_VALUE may not be set if the array has unknown length.  */
> >  if (!max)
> >    {
> >      /* zero sized arrays are represented from C FE as complete types 
> > with
> >         NULL TYPE_MAX_VALUE and zero TYPE_SIZE, while C++ FE represents
> >         them as min 0, max -1.  */
> >      if (COMPLETE_TYPE_P (type)
> >          && integer_zerop (TYPE_SIZE (type))
> >          && integer_zerop (min))
> >        return build_int_cst (TREE_TYPE (min), -1);
> 
> Thanks for the info, I will check on the above, and rewrite “one_element_array_p” with “array_type_nelts”.
> 
> Qing
> > 
> > Richard.
> > 
> >> Thanks a lot for your comments.
> >> 
> >> Qing
> >>> 
> >>>> +  return false;
> >>>> +}
> >>>> +
> >>>> /* Determine whether TYPE is a structure with a flexible array member,
> >>>>  or a union containing such a structure (possibly recursively).  */
> >>>> 
> >>>> @@ -14367,10 +14416,7 @@ flexible_array_type_p (const_tree type)
> >>>> 	  last = x;
> >>>>     if (last == NULL_TREE)
> >>>> 	return false;
> >>>> -      if (TREE_CODE (TREE_TYPE (last)) == ARRAY_TYPE
> >>>> -	  && TYPE_SIZE (TREE_TYPE (last)) == NULL_TREE
> >>>> -	  && TYPE_DOMAIN (TREE_TYPE (last)) != NULL_TREE
> >>>> -	  && TYPE_MAX_VALUE (TYPE_DOMAIN (TREE_TYPE (last))) == NULL_TREE)
> >>>> +      if (flexible_array_member_p (TREE_TYPE (last)))
> >>>> 	return true;
> >>>>     return false;
> >>>>   case UNION_TYPE:
> >>>> diff --git a/gcc/tree.h b/gcc/tree.h
> >>>> index e6564aaccb7..3107de5b499 100644
> >>>> --- a/gcc/tree.h
> >>>> +++ b/gcc/tree.h
> >>>> @@ -2993,6 +2993,11 @@ extern void decl_value_expr_insert (tree, tree);
> >>>> #define DECL_PADDING_P(NODE) \
> >>>> (FIELD_DECL_CHECK (NODE)->decl_common.decl_flag_3)
> >>>> 
> >>>> +/* Used in a FIELD_DECL to indicate whether this field is not a flexible
> >>>> +   array member.  */
> >>>> +#define DECL_NOT_FLEXARRAY(NODE) \
> >>>> +  (FIELD_DECL_CHECK (NODE)->decl_common.decl_not_flexarray)
> >>>> +
> >>>> /* A numeric unique identifier for a LABEL_DECL.  The UID allocation is
> >>>>  dense, unique within any one function, and may be used to index arrays.
> >>>>  If the value is -1, then no UID has been assigned.  */
> >>>> @@ -5531,10 +5536,10 @@ extern tree component_ref_field_offset (tree);
> >>>>  returns null.  */
> >>>> enum struct special_array_member
> >>>> {
> >>>> -   none,      /* Not a special array member.  */
> >>>> -   int_0,     /* Interior array member with size zero.  */
> >>>> -   trail_0,   /* Trailing array member with size zero.  */
> >>>> -   trail_1    /* Trailing array member with one element.  */
> >>>> +    none,	/* Not a special array member.  */
> >>>> +    int_0,	/* Interior array member with size zero.  */
> >>>> +    trail_0,	/* Trailing array member with size zero.  */
> >>>> +    trail_1	/* Trailing array member with one element.  */
> >>>> };
> >>>> 
> >>>> /* Return the size of the member referenced by the COMPONENT_REF, using
> >>>> @@ -6489,6 +6494,9 @@ extern void gt_pch_nx (tree &, gt_pointer_operator, void *);
> >>>> extern bool nonnull_arg_p (const_tree);
> >>>> extern bool is_empty_type (const_tree);
> >>>> extern bool default_is_empty_record (const_tree);
> >>>> +extern bool zero_length_array_p (const_tree);
> >>>> +extern bool one_element_array_p (const_tree, bool * = NULL);
> >>>> +extern bool flexible_array_member_p (const_tree);
> >>>> extern bool flexible_array_type_p (const_tree);
> >>>> extern HOST_WIDE_INT arg_int_size_in_bytes (const_tree);
> >>>> extern tree arg_size_in_bytes (const_tree);
> >>>> 
> >>> 
> >>> -- 
> >>> Richard Biener <rguenther@suse.de>
> >>> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> >>> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> >>> HRB 36809 (AG Nuernberg)
> >> 
> >> 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> > HRB 36809 (AG Nuernberg)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)

  reply	other threads:[~2022-08-02  7:03 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-19 14:09 Qing Zhao
2022-07-27 22:39 ` Kees Cook
2022-07-28  7:26 ` Richard Biener
2022-07-29  5:44   ` Kees Cook
2022-07-29  6:20     ` Richard Biener
2022-07-29 19:56   ` Qing Zhao
2022-08-01  7:38     ` Richard Biener
2022-08-01 15:32       ` Qing Zhao
2022-08-02  7:03         ` Richard Biener [this message]
2022-08-02 14:06           ` Qing Zhao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.YFH.7.77.849.2208020649420.4208@jbgna.fhfr.qr \
    --to=rguenther@suse.de \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jakub@redhat.com \
    --cc=joseph@codesourcery.com \
    --cc=keescook@chromium.org \
    --cc=msebor@gmail.com \
    --cc=qing.zhao@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).