From: Qing Zhao <qing.zhao@oracle.com>
To: Richard Biener <rguenther@suse.de>
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 14:06:46 +0000 [thread overview]
Message-ID: <393DFE88-B99B-41BF-A770-1B177B0E2ACE@oracle.com> (raw)
In-Reply-To: <nycvar.YFH.7.77.849.2208020649420.4208@jbgna.fhfr.qr>
> On Aug 2, 2022, at 3:03 AM, Richard Biener <rguenther@suse.de> wrote:
>
> 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)
Yes, this is on my to-do list, I will finish this part in GCC13. The following is my plan:
=====
1. Add a new IR flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1], [ ] and the option -fstrict-flex-arrays, the attribute strict_flex_arrays and whether it’s the last field of the DECL_CONTEXT.
3. In Middle end, update all places that treat any trailing array as flexible array member with the new flag DECL_NOT_FLEXARRAY + array_at_struct_end_p to control the behavior with multiple levels consistently.
3.1. update tree-object-size.cc to fix PR101836;
3.2. update other places to make GCC consistently handle flexible array member;
4. In Middle end, add -Wstrict-flex-array and update the warnings emitted by -Warray-bounds based on multiple levels;
5. Add a new -Wzero-length-array warning; (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94428).
=====
Let me know if you have any comment or suggestion here.
>
>>
>>>
>>>>>
>>>>>> +
>>>>>> +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.
Thanks, will check on this and confirm with Nathan.
>
>>>
>>>>>
>>>>>>
>>>>>> /* 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)
Okay, I see. Thanks for the detailed explanation.
And now I understand your point, GCC has multiple FEs, so the FE specific concept
(for example, this flexible array member concept is a C/C++ specific concept) should
not be put into middle end.
I will keep this in mind in the next version.
>
>>>
>>> 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.
Okay, will keep the APIs as simple as 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.
Agreed now. -:)
>
>>
>>>
>>> /* 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.
Okay. I see.
>
>>>
>>> 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.
Okay, I see now.
Thanks. Will work on the next version.
Qing
>
>>>
>>>>
>>>>>
>>>>>> + 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)
prev parent reply other threads:[~2022-08-02 14:06 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
2022-08-02 14:06 ` Qing Zhao [this message]
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=393DFE88-B99B-41BF-A770-1B177B0E2ACE@oracle.com \
--to=qing.zhao@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=joseph@codesourcery.com \
--cc=keescook@chromium.org \
--cc=msebor@gmail.com \
--cc=rguenther@suse.de \
/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).