* [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size
@ 2023-02-24 18:35 Qing Zhao
2023-02-24 18:35 ` [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] Qing Zhao
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Qing Zhao @ 2023-02-24 18:35 UTC (permalink / raw)
To: joseph, rguenther; +Cc: siddhesh, keescook, gcc-patches, Qing Zhao
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2460 bytes --]
Hi, Joseph and Richard,
Could you please review this patch and let me know whether it’s ready
for committing into GCC13?
The fix to Bug PR101832 is an important patch for kernel security
purpose. it's better to be put into GCC13.
=============================
These are the 4th version of the patches for PR101832, to fix
builtin_object_size to correctly handle component_ref to a
structure/union field that includes a flexible array member.
also includes a documentation update for the GCC extension on embedding
a structure/union with flexible array member into another structure.
which includes a fix to PR77650.
compared to the 3rd version of the patch, the major changes are:
1. update the documentation part per Joseph's comments.
compared to the 2nd version of the patch, the major changes are:
1. only include C99 flexible array member to this extension, trailing [0], [1]
and [4] are all excluded.
2. for the new bit type_include_flexarray in tree_type_common, print it
and also stream in/out it.
3. update testing cases.
4. more clarification on the documentation. warnings for deprecating the
case when the structure with C99 FAM is embedded in the middle of
another structure.
5. add a new warning option -Wgnu-variable-sized-type-not-at-end for
identifing all such cases.
bootstrapped and regression tested on aarch64 and x86.
Okay for commit?
thanks.
Qing
Qing Zhao (2):
Handle component_ref to a structre/union field including C99 FAM
[PR101832]
Update documentation to clarify a GCC extension
gcc/c-family/c.opt | 5 +
gcc/c/c-decl.cc | 19 +++
gcc/cp/module.cc | 2 +
gcc/doc/extend.texi | 48 ++++++-
gcc/print-tree.cc | 5 +
.../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
.../gcc.dg/variable-sized-type-flex-array.c | 31 ++++
gcc/tree-core.h | 4 +-
gcc/tree-object-size.cc | 79 +++++++----
gcc/tree-streamer-in.cc | 1 +
gcc/tree-streamer-out.cc | 1 +
gcc/tree.h | 6 +
12 files changed, 305 insertions(+), 30 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
create mode 100644 gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
--
2.31.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-02-24 18:35 [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
@ 2023-02-24 18:35 ` Qing Zhao
2023-03-03 0:03 ` Qing Zhao
2023-03-09 12:20 ` Richard Biener
2023-02-24 18:35 ` [V4][PATCH 2/2] Update documentation to clarify a GCC extension Qing Zhao
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Qing Zhao @ 2023-02-24 18:35 UTC (permalink / raw)
To: joseph, rguenther; +Cc: siddhesh, keescook, gcc-patches, Qing Zhao
GCC extension accepts the case when a struct with a C99 flexible array member
is embedded into another struct or union (possibly recursively).
__builtin_object_size should treat such struct as flexible size.
gcc/c/ChangeLog:
PR tree-optimization/101832
* c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
struct/union type.
gcc/cp/ChangeLog:
PR tree-optimization/101832
* module.cc (trees_out::core_bools): Stream out new bit
type_include_flexarray.
(trees_in::core_bools): Stream in new bit type_include_flexarray.
gcc/ChangeLog:
PR tree-optimization/101832
* print-tree.cc (print_node): Print new bit type_include_flexarray.
* tree-core.h (struct tree_type_common): New bit
type_include_flexarray.
* tree-object-size.cc (addr_object_size): Handle structure/union type
when it has flexible size.
* tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
in new bit type_include_flexarray.
* tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
out new bit type_include_flexarray.
* tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
TYPE_INCLUDE_FLEXARRAY.
gcc/testsuite/ChangeLog:
PR tree-optimization/101832
* gcc.dg/builtin-object-size-pr101832.c: New test.
---
gcc/c/c-decl.cc | 12 ++
gcc/cp/module.cc | 2 +
gcc/print-tree.cc | 5 +
.../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
gcc/tree-core.h | 4 +-
gcc/tree-object-size.cc | 79 +++++++----
gcc/tree-streamer-in.cc | 1 +
gcc/tree-streamer-out.cc | 1 +
gcc/tree.h | 6 +
9 files changed, 215 insertions(+), 29 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 08078eadeb8..f589a2f5192 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
/* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
+ /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
+ * when x is an array. */
+ if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
+ TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
+ /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
+ when x is the last field. */
+ else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
+ || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
+ && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
+ && is_last_field)
+ TYPE_INCLUDE_FLEXARRAY (t) = true;
+
if (DECL_NAME (x)
|| RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
saw_named_field = true;
diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
index ac2fe66b080..c750361b704 100644
--- a/gcc/cp/module.cc
+++ b/gcc/cp/module.cc
@@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
WB (t->type_common.lang_flag_5);
WB (t->type_common.lang_flag_6);
WB (t->type_common.typeless_storage);
+ WB (t->type_common.type_include_flexarray);
}
if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
@@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
RB (t->type_common.lang_flag_5);
RB (t->type_common.lang_flag_6);
RB (t->type_common.typeless_storage);
+ RB (t->type_common.type_include_flexarray);
}
if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
index 1f3afcbbc86..efacdb7686f 100644
--- a/gcc/print-tree.cc
+++ b/gcc/print-tree.cc
@@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
&& TYPE_CXX_ODR_P (node))
fputs (" cxx-odr-p", file);
+ if ((code == RECORD_TYPE
+ || code == UNION_TYPE)
+ && TYPE_INCLUDE_FLEXARRAY (node))
+ fputs (" include-flexarray", file);
+
/* The transparent-union flag is used for different things in
different nodes. */
if ((code == UNION_TYPE || code == RECORD_TYPE)
diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
new file mode 100644
index 00000000000..60078e11634
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
@@ -0,0 +1,134 @@
+/* PR 101832:
+ GCC extension accepts the case when a struct with a C99 flexible array
+ member is embedded into another struct (possibly recursively).
+ __builtin_object_size will treat such struct as flexible size.
+ However, when a structure with non-C99 flexible array member, i.e, trailing
+ [0], [1], or [4], is embedded into anther struct, the stucture will not
+ be treated as flexible size. */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#include "builtin-object-size-common.h"
+
+#define expect(p, _v) do { \
+ size_t v = _v; \
+ if (p == v) \
+ __builtin_printf ("ok: %s == %zd\n", #p, p); \
+ else {\
+ __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+ FAIL (); \
+ } \
+} while (0);
+
+
+struct A {
+ int n;
+ char data[];
+};
+
+struct B {
+ int m;
+ struct A a;
+};
+
+struct C {
+ int q;
+ struct B b;
+};
+
+struct A0 {
+ int n;
+ char data[0];
+};
+
+struct B0 {
+ int m;
+ struct A0 a;
+};
+
+struct C0 {
+ int q;
+ struct B0 b;
+};
+
+struct A1 {
+ int n;
+ char data[1];
+};
+
+struct B1 {
+ int m;
+ struct A1 a;
+};
+
+struct C1 {
+ int q;
+ struct B1 b;
+};
+
+struct An {
+ int n;
+ char data[8];
+};
+
+struct Bn {
+ int m;
+ struct An a;
+};
+
+struct Cn {
+ int q;
+ struct Bn b;
+};
+
+volatile void *magic1, *magic2;
+
+int main (int argc, char *argv[])
+{
+ struct B *outer;
+ struct C *outest;
+
+ /* Make sure optimization can't find some other object size. */
+ outer = (void *)magic1;
+ outest = (void *)magic2;
+
+ expect (__builtin_object_size (&outer->a, 1), -1);
+ expect (__builtin_object_size (&outest->b, 1), -1);
+ expect (__builtin_object_size (&outest->b.a, 1), -1);
+
+ struct B0 *outer0;
+ struct C0 *outest0;
+
+ /* Make sure optimization can't find some other object size. */
+ outer0 = (void *)magic1;
+ outest0 = (void *)magic2;
+
+ expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
+ expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
+ expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
+
+ struct B1 *outer1;
+ struct C1 *outest1;
+
+ /* Make sure optimization can't find some other object size. */
+ outer1 = (void *)magic1;
+ outest1 = (void *)magic2;
+
+ expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
+ expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
+ expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
+
+ struct Bn *outern;
+ struct Cn *outestn;
+
+ /* Make sure optimization can't find some other object size. */
+ outern = (void *)magic1;
+ outestn = (void *)magic2;
+
+ expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
+ expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
+ expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
+
+ DONE ();
+ return 0;
+}
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index acd8deea34e..705d5702b9c 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
unsigned empty_flag : 1;
unsigned indivisible_p : 1;
unsigned no_named_args_stdarg_p : 1;
- unsigned spare : 15;
+ /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
+ unsigned int type_include_flexarray : 1;
+ unsigned spare : 14;
alias_set_type alias_set;
tree pointer_to;
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index 9a936a91983..22b3c72ea6e 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
v = NULL_TREE;
break;
case COMPONENT_REF:
- if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
+ /* When the ref is not to an array, a record or a union, it
+ will not have flexible size, compute the object size
+ directly. */
+ if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
+ && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
+ && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
{
v = NULL_TREE;
break;
}
- is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
- while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
- if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
- != UNION_TYPE
- && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
- != QUAL_UNION_TYPE)
- break;
- else
- v = TREE_OPERAND (v, 0);
- if (TREE_CODE (v) == COMPONENT_REF
- && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
- == RECORD_TYPE)
+ if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
+ || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
+ /* if the record or union does not include a flexible array
+ recursively, compute the object size directly. */
{
- /* compute object size only if v is not a
- flexible array member. */
- if (!is_flexible_array_mem_ref)
+ if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
{
v = NULL_TREE;
break;
}
- v = TREE_OPERAND (v, 0);
+ else
+ v = TREE_OPERAND (v, 0);
}
- while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
- if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
- != UNION_TYPE
- && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
- != QUAL_UNION_TYPE)
- break;
- else
- v = TREE_OPERAND (v, 0);
- if (v != pt_var)
- v = NULL_TREE;
else
- v = pt_var;
+ {
+ /* Now the ref is to an array type. */
+ is_flexible_array_mem_ref
+ = array_ref_flexible_size_p (v);
+ while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
+ if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
+ != UNION_TYPE
+ && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
+ != QUAL_UNION_TYPE)
+ break;
+ else
+ v = TREE_OPERAND (v, 0);
+ if (TREE_CODE (v) == COMPONENT_REF
+ && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
+ == RECORD_TYPE)
+ {
+ /* compute object size only if v is not a
+ flexible array member. */
+ if (!is_flexible_array_mem_ref)
+ {
+ v = NULL_TREE;
+ break;
+ }
+ v = TREE_OPERAND (v, 0);
+ }
+ while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
+ if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
+ != UNION_TYPE
+ && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
+ != QUAL_UNION_TYPE)
+ break;
+ else
+ v = TREE_OPERAND (v, 0);
+ if (v != pt_var)
+ v = NULL_TREE;
+ else
+ v = pt_var;
+ }
break;
default:
v = pt_var;
diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
index d4dc30f048f..c19ede0631d 100644
--- a/gcc/tree-streamer-in.cc
+++ b/gcc/tree-streamer-in.cc
@@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
+ TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
}
else if (TREE_CODE (expr) == ARRAY_TYPE)
TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
index d107229da5c..73e4b4e547c 100644
--- a/gcc/tree-streamer-out.cc
+++ b/gcc/tree-streamer-out.cc
@@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
: TYPE_CXX_ODR_P (expr), 1);
+ bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
}
else if (TREE_CODE (expr) == ARRAY_TYPE)
bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
diff --git a/gcc/tree.h b/gcc/tree.h
index 92ac0e6a214..ab1cdc3dc85 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
#define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
(TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
+/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
+ at the last field recursively. */
+#define TYPE_INCLUDE_FLEXARRAY(NODE) \
+ (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
+
+
/* In an IDENTIFIER_NODE, this means that assemble_name was called with
this string as an argument. */
#define TREE_SYMBOL_REFERENCED(NODE) \
--
2.31.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [V4][PATCH 2/2] Update documentation to clarify a GCC extension
2023-02-24 18:35 [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-02-24 18:35 ` [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] Qing Zhao
@ 2023-02-24 18:35 ` Qing Zhao
2023-03-03 0:03 ` Qing Zhao
2023-03-15 3:26 ` Sandra Loosemore
2023-03-03 0:02 ` [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-03-08 18:45 ` Kees Cook
3 siblings, 2 replies; 18+ messages in thread
From: Qing Zhao @ 2023-02-24 18:35 UTC (permalink / raw)
To: joseph, rguenther; +Cc: siddhesh, keescook, gcc-patches, Qing Zhao
on a structure with a C99 flexible array member being nested in
another structure.
"GCC extension accepts a structure containing an ISO C99 "flexible array
member", or a union containing such a structure (possibly recursively)
to be a member of a structure.
There are two situations:
* The structure with a C99 flexible array member is the last field of
another structure, for example:
struct flex { int length; char data[]; };
union union_flex { int others; struct flex f; };
struct out_flex_struct { int m; struct flex flex_data; };
struct out_flex_union { int n; union union_flex flex_data; };
In the above, both 'out_flex_struct.flex_data.data[]' and
'out_flex_union.flex_data.f.data[]' are considered as flexible
arrays too.
* The structure with a C99 flexible array member is the middle field
of another structure, for example:
struct flex { int length; char data[]; };
struct mid_flex { int m; struct flex flex_data; int n; };
In the above, 'mid_flex.flex_data.data[]' is allowed to be extended
flexibly to the padding. E.g, up to 4 elements.
However, relying on space in struct padding is a bad programming
practice, compilers do not handle such extension consistently, Any
code relying on this behavior should be modified to ensure that
flexible array members only end up at the ends of structures.
Please use warning option '-Wgnu-variable-sized-type-not-at-end' to
identify all such cases in the source code and modify them. This
extension will be deprecated from gcc in the next release.
"
gcc/c-family/ChangeLog:
* c.opt: New option -Wgnu-variable-sized-type-not-at-end.
gcc/c/ChangeLog:
* c-decl.cc (finish_struct): Issue warnings for new option.
gcc/ChangeLog:
* doc/extend.texi: Document GCC extension on a structure containing
a flexible array member to be a member of another structure.
gcc/testsuite/ChangeLog:
* gcc.dg/variable-sized-type-flex-array.c: New test.
---
gcc/c-family/c.opt | 5 ++
gcc/c/c-decl.cc | 7 +++
gcc/doc/extend.texi | 48 ++++++++++++++++++-
.../gcc.dg/variable-sized-type-flex-array.c | 31 ++++++++++++
4 files changed, 90 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 3333cddeece..660ac07f3d4 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -737,6 +737,11 @@ Wformat-truncation=
C ObjC C++ LTO ObjC++ Joined RejectNegative UInteger Var(warn_format_trunc) Warning LangEnabledBy(C ObjC C++ LTO ObjC++,Wformat=, warn_format >= 1, 0) IntegerRange(0, 2)
Warn about calls to snprintf and similar functions that truncate output.
+Wgnu-variable-sized-type-not-at-end
+C C++ Var(warn_variable_sized_type_not_at_end) Warning
+Warn about structures or unions with C99 flexible array members are not
+at the end of a structure.
+
Wif-not-aligned
C ObjC C++ ObjC++ Var(warn_if_not_aligned) Init(1) Warning
Warn when the field in a struct is not aligned.
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index f589a2f5192..c5b54f07965 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -9296,6 +9296,13 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
&& is_last_field)
TYPE_INCLUDE_FLEXARRAY (t) = true;
+ if (warn_variable_sized_type_not_at_end
+ && !is_last_field
+ && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x)))
+ warning_at (DECL_SOURCE_LOCATION (x),
+ OPT_Wgnu_variable_sized_type_not_at_end,
+ "variable sized type not at the end of a struct");
+
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 c1122916255..e278148c332 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -1748,7 +1748,53 @@ Flexible array members may only appear as the last member of a
A structure containing a flexible array member, or a union containing
such a structure (possibly recursively), may not be a member of a
structure or an element of an array. (However, these uses are
-permitted by GCC as extensions.)
+permitted by GCC as extensions, see details below.)
+@end itemize
+
+GCC extension accepts a structure containing an ISO C99 @dfn{flexible array
+member}, or a union containing such a structure (possibly recursively)
+to be a member of a structure.
+
+There are two situations:
+
+@itemize @bullet
+@item
+The structure with a C99 flexible array member is the last field of another
+structure, for example:
+
+@smallexample
+struct flex @{ int length; char data[]; @};
+union union_flex @{ int others; struct flex f; @};
+
+struct out_flex_struct @{ int m; struct flex flex_data; @};
+struct out_flex_union @{ int n; union union_flex flex_data; @};
+@end smallexample
+
+In the above, both @code{out_flex_struct.flex_data.data[]} and
+@code{out_flex_union.flex_data.f.data[]} are considered as flexible arrays too.
+
+
+@item
+The structure with a C99 flexible array member is the middle field of another
+structure, for example:
+
+@smallexample
+struct flex @{ int length; char data[]; @};
+
+struct mid_flex @{ int m; struct flex flex_data; int n; @};
+@end smallexample
+
+In the above, @code{mid_flex.flex_data.data[]} is allowed to be extended
+flexibly to the padding. E.g, up to 4 elements.
+
+However, relying on space in struct padding is a bad programming practice,
+compilers do not handle such extension consistently, Any code relying on
+this behavior should be modified to ensure that flexible array members
+only end up at the ends of structures.
+
+Please use warning option @option{-Wgnu-variable-sized-type-not-at-end} to
+identify all such cases in the source code and modify them. This extension
+will be deprecated from gcc in the next release.
@end itemize
Non-empty initialization of zero-length
diff --git a/gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c b/gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
new file mode 100644
index 00000000000..e3f65c5ed07
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
@@ -0,0 +1,31 @@
+/* Test for -Wgnu-variable-sized-type-not-at-end on structure/union with
+ C99 flexible array members being embedded into another structure. */
+/* { dg-do compile } */
+/* { dg-options "-Wgnu-variable-sized-type-not-at-end" } */
+
+struct flex { int n; int data[]; };
+struct out_flex_end { int m; struct flex flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+struct out_flex_mid { struct flex flex_data; int m; }; /* { dg-warning "variable sized type not at the end of a struct" } */
+/* since the warning has been issued for out_flex_mid, no need to
+ issue warning again when it is included in another structure/union. */
+struct outer_flex_mid { struct out_flex_mid out_flex_data; int p; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+union flex_union_mid { int a; struct outer_flex_mid b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+
+
+struct flex0 { int n; int data[0]; };
+struct out_flex_end0 { int m; struct flex0 flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+struct out_flex_mid0 { struct flex0 flex_data; int m; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+struct outer_flex_mid0 { struct out_flex_mid0 out_flex_data; int p; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+union flex_union_mid0 { int a; struct outer_flex_mid0 b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+
+struct flex1 { int n; int data[1]; };
+struct out_flex_end1 { int m; struct flex1 flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+struct out_flex_mid1 { struct flex1 flex_data; int m; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+struct outer_flex_mid1 { struct out_flex_mid1 out_flex_data; int p; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+union flex_union_mid1 { int a; struct outer_flex_mid1 b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+
+struct flexn { int n; int data[8]; };
+struct out_flex_endn { int m; struct flexn flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
+struct out_flex_midn { struct flexn flex_data; int m; }; /* { dg-bogus"variable sized type not at the end of a struct" } */
+struct outer_flex_midn { struct out_flex_midn out_flex_data; int p; }; /* { dg-bogus"variable sized type not at the end of a struct" } */
+union flex_union_midn { int a; struct outer_flex_midn b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
--
2.31.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size
2023-02-24 18:35 [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-02-24 18:35 ` [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] Qing Zhao
2023-02-24 18:35 ` [V4][PATCH 2/2] Update documentation to clarify a GCC extension Qing Zhao
@ 2023-03-03 0:02 ` Qing Zhao
2023-03-08 18:45 ` Kees Cook
3 siblings, 0 replies; 18+ messages in thread
From: Qing Zhao @ 2023-03-03 0:02 UTC (permalink / raw)
To: Joseph Myers, Richard Biener; +Cc: siddhesh, keescook, gcc-patches
Ping
Qing
> On Feb 24, 2023, at 1:35 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
>
> Hi, Joseph and Richard,
>
> Could you please review this patch and let me know whether it’s ready
> for committing into GCC13?
>
> The fix to Bug PR101832 is an important patch for kernel security
> purpose. it's better to be put into GCC13.
>
> =============================
>
> These are the 4th version of the patches for PR101832, to fix
> builtin_object_size to correctly handle component_ref to a
> structure/union field that includes a flexible array member.
>
> also includes a documentation update for the GCC extension on embedding
> a structure/union with flexible array member into another structure.
> which includes a fix to PR77650.
>
> compared to the 3rd version of the patch, the major changes are:
>
> 1. update the documentation part per Joseph's comments.
>
> compared to the 2nd version of the patch, the major changes are:
>
> 1. only include C99 flexible array member to this extension, trailing [0], [1]
> and [4] are all excluded.
> 2. for the new bit type_include_flexarray in tree_type_common, print it
> and also stream in/out it.
> 3. update testing cases.
> 4. more clarification on the documentation. warnings for deprecating the
> case when the structure with C99 FAM is embedded in the middle of
> another structure.
> 5. add a new warning option -Wgnu-variable-sized-type-not-at-end for
> identifing all such cases.
>
> bootstrapped and regression tested on aarch64 and x86.
>
> Okay for commit?
>
> thanks.
>
> Qing
>
> Qing Zhao (2):
> Handle component_ref to a structre/union field including C99 FAM
> [PR101832]
> Update documentation to clarify a GCC extension
>
> gcc/c-family/c.opt | 5 +
> gcc/c/c-decl.cc | 19 +++
> gcc/cp/module.cc | 2 +
> gcc/doc/extend.texi | 48 ++++++-
> gcc/print-tree.cc | 5 +
> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
> .../gcc.dg/variable-sized-type-flex-array.c | 31 ++++
> gcc/tree-core.h | 4 +-
> gcc/tree-object-size.cc | 79 +++++++----
> gcc/tree-streamer-in.cc | 1 +
> gcc/tree-streamer-out.cc | 1 +
> gcc/tree.h | 6 +
> 12 files changed, 305 insertions(+), 30 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> create mode 100644 gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-02-24 18:35 ` [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] Qing Zhao
@ 2023-03-03 0:03 ` Qing Zhao
2023-03-09 12:20 ` Richard Biener
1 sibling, 0 replies; 18+ messages in thread
From: Qing Zhao @ 2023-03-03 0:03 UTC (permalink / raw)
To: Joseph Myers, Richard Biener; +Cc: siddhesh, keescook, gcc-patches
Ping.
Qing
> On Feb 24, 2023, at 1:35 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
>
> GCC extension accepts the case when a struct with a C99 flexible array member
> is embedded into another struct or union (possibly recursively).
> __builtin_object_size should treat such struct as flexible size.
>
> gcc/c/ChangeLog:
>
> PR tree-optimization/101832
> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
> struct/union type.
>
> gcc/cp/ChangeLog:
>
> PR tree-optimization/101832
> * module.cc (trees_out::core_bools): Stream out new bit
> type_include_flexarray.
> (trees_in::core_bools): Stream in new bit type_include_flexarray.
>
> gcc/ChangeLog:
>
> PR tree-optimization/101832
> * print-tree.cc (print_node): Print new bit type_include_flexarray.
> * tree-core.h (struct tree_type_common): New bit
> type_include_flexarray.
> * tree-object-size.cc (addr_object_size): Handle structure/union type
> when it has flexible size.
> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
> in new bit type_include_flexarray.
> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
> out new bit type_include_flexarray.
> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
> TYPE_INCLUDE_FLEXARRAY.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/101832
> * gcc.dg/builtin-object-size-pr101832.c: New test.
> ---
> gcc/c/c-decl.cc | 12 ++
> gcc/cp/module.cc | 2 +
> gcc/print-tree.cc | 5 +
> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
> gcc/tree-core.h | 4 +-
> gcc/tree-object-size.cc | 79 +++++++----
> gcc/tree-streamer-in.cc | 1 +
> gcc/tree-streamer-out.cc | 1 +
> gcc/tree.h | 6 +
> 9 files changed, 215 insertions(+), 29 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>
> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> index 08078eadeb8..f589a2f5192 100644
> --- a/gcc/c/c-decl.cc
> +++ b/gcc/c/c-decl.cc
> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
>
> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> + * when x is an array. */
> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> + when x is the last field. */
> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
> + && is_last_field)
> + TYPE_INCLUDE_FLEXARRAY (t) = true;
> +
> if (DECL_NAME (x)
> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> saw_named_field = true;
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index ac2fe66b080..c750361b704 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
> WB (t->type_common.lang_flag_5);
> WB (t->type_common.lang_flag_6);
> WB (t->type_common.typeless_storage);
> + WB (t->type_common.type_include_flexarray);
> }
>
> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
> RB (t->type_common.lang_flag_5);
> RB (t->type_common.lang_flag_6);
> RB (t->type_common.typeless_storage);
> + RB (t->type_common.type_include_flexarray);
> }
>
> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
> index 1f3afcbbc86..efacdb7686f 100644
> --- a/gcc/print-tree.cc
> +++ b/gcc/print-tree.cc
> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
> && TYPE_CXX_ODR_P (node))
> fputs (" cxx-odr-p", file);
>
> + if ((code == RECORD_TYPE
> + || code == UNION_TYPE)
> + && TYPE_INCLUDE_FLEXARRAY (node))
> + fputs (" include-flexarray", file);
> +
> /* The transparent-union flag is used for different things in
> different nodes. */
> if ((code == UNION_TYPE || code == RECORD_TYPE)
> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> new file mode 100644
> index 00000000000..60078e11634
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> @@ -0,0 +1,134 @@
> +/* PR 101832:
> + GCC extension accepts the case when a struct with a C99 flexible array
> + member is embedded into another struct (possibly recursively).
> + __builtin_object_size will treat such struct as flexible size.
> + However, when a structure with non-C99 flexible array member, i.e, trailing
> + [0], [1], or [4], is embedded into anther struct, the stucture will not
> + be treated as flexible size. */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +#include "builtin-object-size-common.h"
> +
> +#define expect(p, _v) do { \
> + size_t v = _v; \
> + if (p == v) \
> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
> + else {\
> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
> + FAIL (); \
> + } \
> +} while (0);
> +
> +
> +struct A {
> + int n;
> + char data[];
> +};
> +
> +struct B {
> + int m;
> + struct A a;
> +};
> +
> +struct C {
> + int q;
> + struct B b;
> +};
> +
> +struct A0 {
> + int n;
> + char data[0];
> +};
> +
> +struct B0 {
> + int m;
> + struct A0 a;
> +};
> +
> +struct C0 {
> + int q;
> + struct B0 b;
> +};
> +
> +struct A1 {
> + int n;
> + char data[1];
> +};
> +
> +struct B1 {
> + int m;
> + struct A1 a;
> +};
> +
> +struct C1 {
> + int q;
> + struct B1 b;
> +};
> +
> +struct An {
> + int n;
> + char data[8];
> +};
> +
> +struct Bn {
> + int m;
> + struct An a;
> +};
> +
> +struct Cn {
> + int q;
> + struct Bn b;
> +};
> +
> +volatile void *magic1, *magic2;
> +
> +int main (int argc, char *argv[])
> +{
> + struct B *outer;
> + struct C *outest;
> +
> + /* Make sure optimization can't find some other object size. */
> + outer = (void *)magic1;
> + outest = (void *)magic2;
> +
> + expect (__builtin_object_size (&outer->a, 1), -1);
> + expect (__builtin_object_size (&outest->b, 1), -1);
> + expect (__builtin_object_size (&outest->b.a, 1), -1);
> +
> + struct B0 *outer0;
> + struct C0 *outest0;
> +
> + /* Make sure optimization can't find some other object size. */
> + outer0 = (void *)magic1;
> + outest0 = (void *)magic2;
> +
> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
> +
> + struct B1 *outer1;
> + struct C1 *outest1;
> +
> + /* Make sure optimization can't find some other object size. */
> + outer1 = (void *)magic1;
> + outest1 = (void *)magic2;
> +
> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
> +
> + struct Bn *outern;
> + struct Cn *outestn;
> +
> + /* Make sure optimization can't find some other object size. */
> + outern = (void *)magic1;
> + outestn = (void *)magic2;
> +
> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
> +
> + DONE ();
> + return 0;
> +}
> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index acd8deea34e..705d5702b9c 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
> unsigned empty_flag : 1;
> unsigned indivisible_p : 1;
> unsigned no_named_args_stdarg_p : 1;
> - unsigned spare : 15;
> + /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
> + unsigned int type_include_flexarray : 1;
> + unsigned spare : 14;
>
> alias_set_type alias_set;
> tree pointer_to;
> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> index 9a936a91983..22b3c72ea6e 100644
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
> v = NULL_TREE;
> break;
> case COMPONENT_REF:
> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> + /* When the ref is not to an array, a record or a union, it
> + will not have flexible size, compute the object size
> + directly. */
> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
> {
> v = NULL_TREE;
> break;
> }
> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != UNION_TYPE
> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != QUAL_UNION_TYPE)
> - break;
> - else
> - v = TREE_OPERAND (v, 0);
> - if (TREE_CODE (v) == COMPONENT_REF
> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - == RECORD_TYPE)
> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
> + /* if the record or union does not include a flexible array
> + recursively, compute the object size directly. */
> {
> - /* compute object size only if v is not a
> - flexible array member. */
> - if (!is_flexible_array_mem_ref)
> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> {
> v = NULL_TREE;
> break;
> }
> - v = TREE_OPERAND (v, 0);
> + else
> + v = TREE_OPERAND (v, 0);
> }
> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != UNION_TYPE
> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != QUAL_UNION_TYPE)
> - break;
> - else
> - v = TREE_OPERAND (v, 0);
> - if (v != pt_var)
> - v = NULL_TREE;
> else
> - v = pt_var;
> + {
> + /* Now the ref is to an array type. */
> + is_flexible_array_mem_ref
> + = array_ref_flexible_size_p (v);
> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != UNION_TYPE
> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != QUAL_UNION_TYPE)
> + break;
> + else
> + v = TREE_OPERAND (v, 0);
> + if (TREE_CODE (v) == COMPONENT_REF
> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + == RECORD_TYPE)
> + {
> + /* compute object size only if v is not a
> + flexible array member. */
> + if (!is_flexible_array_mem_ref)
> + {
> + v = NULL_TREE;
> + break;
> + }
> + v = TREE_OPERAND (v, 0);
> + }
> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != UNION_TYPE
> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != QUAL_UNION_TYPE)
> + break;
> + else
> + v = TREE_OPERAND (v, 0);
> + if (v != pt_var)
> + v = NULL_TREE;
> + else
> + v = pt_var;
> + }
> break;
> default:
> v = pt_var;
> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
> index d4dc30f048f..c19ede0631d 100644
> --- a/gcc/tree-streamer-in.cc
> +++ b/gcc/tree-streamer-in.cc
> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
> }
> else if (TREE_CODE (expr) == ARRAY_TYPE)
> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
> index d107229da5c..73e4b4e547c 100644
> --- a/gcc/tree-streamer-out.cc
> +++ b/gcc/tree-streamer-out.cc
> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
> : TYPE_CXX_ODR_P (expr), 1);
> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
> }
> else if (TREE_CODE (expr) == ARRAY_TYPE)
> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
> diff --git a/gcc/tree.h b/gcc/tree.h
> index 92ac0e6a214..ab1cdc3dc85 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>
> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> + at the last field recursively. */
> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
> +
> +
> /* In an IDENTIFIER_NODE, this means that assemble_name was called with
> this string as an argument. */
> #define TREE_SYMBOL_REFERENCED(NODE) \
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [V4][PATCH 2/2] Update documentation to clarify a GCC extension
2023-02-24 18:35 ` [V4][PATCH 2/2] Update documentation to clarify a GCC extension Qing Zhao
@ 2023-03-03 0:03 ` Qing Zhao
2023-03-12 23:14 ` Sandra Loosemore
2023-03-15 3:26 ` Sandra Loosemore
1 sibling, 1 reply; 18+ messages in thread
From: Qing Zhao @ 2023-03-03 0:03 UTC (permalink / raw)
To: Joseph Myers, Richard Biener; +Cc: siddhesh, keescook, gcc-patches
Ping.
Qing
> On Feb 24, 2023, at 1:35 PM, Qing Zhao <qing.zhao@oracle.com> wrote:
>
> on a structure with a C99 flexible array member being nested in
> another structure.
>
> "GCC extension accepts a structure containing an ISO C99 "flexible array
> member", or a union containing such a structure (possibly recursively)
> to be a member of a structure.
>
> There are two situations:
>
> * The structure with a C99 flexible array member is the last field of
> another structure, for example:
>
> struct flex { int length; char data[]; };
> union union_flex { int others; struct flex f; };
>
> struct out_flex_struct { int m; struct flex flex_data; };
> struct out_flex_union { int n; union union_flex flex_data; };
>
> In the above, both 'out_flex_struct.flex_data.data[]' and
> 'out_flex_union.flex_data.f.data[]' are considered as flexible
> arrays too.
>
> * The structure with a C99 flexible array member is the middle field
> of another structure, for example:
>
> struct flex { int length; char data[]; };
>
> struct mid_flex { int m; struct flex flex_data; int n; };
>
> In the above, 'mid_flex.flex_data.data[]' is allowed to be extended
> flexibly to the padding. E.g, up to 4 elements.
>
> However, relying on space in struct padding is a bad programming
> practice, compilers do not handle such extension consistently, Any
> code relying on this behavior should be modified to ensure that
> flexible array members only end up at the ends of structures.
>
> Please use warning option '-Wgnu-variable-sized-type-not-at-end' to
> identify all such cases in the source code and modify them. This
> extension will be deprecated from gcc in the next release.
> "
>
> gcc/c-family/ChangeLog:
>
> * c.opt: New option -Wgnu-variable-sized-type-not-at-end.
>
> gcc/c/ChangeLog:
>
> * c-decl.cc (finish_struct): Issue warnings for new option.
>
> gcc/ChangeLog:
>
> * doc/extend.texi: Document GCC extension on a structure containing
> a flexible array member to be a member of another structure.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/variable-sized-type-flex-array.c: New test.
> ---
> gcc/c-family/c.opt | 5 ++
> gcc/c/c-decl.cc | 7 +++
> gcc/doc/extend.texi | 48 ++++++++++++++++++-
> .../gcc.dg/variable-sized-type-flex-array.c | 31 ++++++++++++
> 4 files changed, 90 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
>
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index 3333cddeece..660ac07f3d4 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -737,6 +737,11 @@ Wformat-truncation=
> C ObjC C++ LTO ObjC++ Joined RejectNegative UInteger Var(warn_format_trunc) Warning LangEnabledBy(C ObjC C++ LTO ObjC++,Wformat=, warn_format >= 1, 0) IntegerRange(0, 2)
> Warn about calls to snprintf and similar functions that truncate output.
>
> +Wgnu-variable-sized-type-not-at-end
> +C C++ Var(warn_variable_sized_type_not_at_end) Warning
> +Warn about structures or unions with C99 flexible array members are not
> +at the end of a structure.
> +
> Wif-not-aligned
> C ObjC C++ ObjC++ Var(warn_if_not_aligned) Init(1) Warning
> Warn when the field in a struct is not aligned.
> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> index f589a2f5192..c5b54f07965 100644
> --- a/gcc/c/c-decl.cc
> +++ b/gcc/c/c-decl.cc
> @@ -9296,6 +9296,13 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> && is_last_field)
> TYPE_INCLUDE_FLEXARRAY (t) = true;
>
> + if (warn_variable_sized_type_not_at_end
> + && !is_last_field
> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x)))
> + warning_at (DECL_SOURCE_LOCATION (x),
> + OPT_Wgnu_variable_sized_type_not_at_end,
> + "variable sized type not at the end of a struct");
> +
> 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 c1122916255..e278148c332 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -1748,7 +1748,53 @@ Flexible array members may only appear as the last member of a
> A structure containing a flexible array member, or a union containing
> such a structure (possibly recursively), may not be a member of a
> structure or an element of an array. (However, these uses are
> -permitted by GCC as extensions.)
> +permitted by GCC as extensions, see details below.)
> +@end itemize
> +
> +GCC extension accepts a structure containing an ISO C99 @dfn{flexible array
> +member}, or a union containing such a structure (possibly recursively)
> +to be a member of a structure.
> +
> +There are two situations:
> +
> +@itemize @bullet
> +@item
> +The structure with a C99 flexible array member is the last field of another
> +structure, for example:
> +
> +@smallexample
> +struct flex @{ int length; char data[]; @};
> +union union_flex @{ int others; struct flex f; @};
> +
> +struct out_flex_struct @{ int m; struct flex flex_data; @};
> +struct out_flex_union @{ int n; union union_flex flex_data; @};
> +@end smallexample
> +
> +In the above, both @code{out_flex_struct.flex_data.data[]} and
> +@code{out_flex_union.flex_data.f.data[]} are considered as flexible arrays too.
> +
> +
> +@item
> +The structure with a C99 flexible array member is the middle field of another
> +structure, for example:
> +
> +@smallexample
> +struct flex @{ int length; char data[]; @};
> +
> +struct mid_flex @{ int m; struct flex flex_data; int n; @};
> +@end smallexample
> +
> +In the above, @code{mid_flex.flex_data.data[]} is allowed to be extended
> +flexibly to the padding. E.g, up to 4 elements.
> +
> +However, relying on space in struct padding is a bad programming practice,
> +compilers do not handle such extension consistently, Any code relying on
> +this behavior should be modified to ensure that flexible array members
> +only end up at the ends of structures.
> +
> +Please use warning option @option{-Wgnu-variable-sized-type-not-at-end} to
> +identify all such cases in the source code and modify them. This extension
> +will be deprecated from gcc in the next release.
> @end itemize
>
> Non-empty initialization of zero-length
> diff --git a/gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c b/gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
> new file mode 100644
> index 00000000000..e3f65c5ed07
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/variable-sized-type-flex-array.c
> @@ -0,0 +1,31 @@
> +/* Test for -Wgnu-variable-sized-type-not-at-end on structure/union with
> + C99 flexible array members being embedded into another structure. */
> +/* { dg-do compile } */
> +/* { dg-options "-Wgnu-variable-sized-type-not-at-end" } */
> +
> +struct flex { int n; int data[]; };
> +struct out_flex_end { int m; struct flex flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +struct out_flex_mid { struct flex flex_data; int m; }; /* { dg-warning "variable sized type not at the end of a struct" } */
> +/* since the warning has been issued for out_flex_mid, no need to
> + issue warning again when it is included in another structure/union. */
> +struct outer_flex_mid { struct out_flex_mid out_flex_data; int p; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +union flex_union_mid { int a; struct outer_flex_mid b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +
> +
> +struct flex0 { int n; int data[0]; };
> +struct out_flex_end0 { int m; struct flex0 flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +struct out_flex_mid0 { struct flex0 flex_data; int m; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +struct outer_flex_mid0 { struct out_flex_mid0 out_flex_data; int p; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +union flex_union_mid0 { int a; struct outer_flex_mid0 b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +
> +struct flex1 { int n; int data[1]; };
> +struct out_flex_end1 { int m; struct flex1 flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +struct out_flex_mid1 { struct flex1 flex_data; int m; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +struct outer_flex_mid1 { struct out_flex_mid1 out_flex_data; int p; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +union flex_union_mid1 { int a; struct outer_flex_mid1 b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +
> +struct flexn { int n; int data[8]; };
> +struct out_flex_endn { int m; struct flexn flex_data; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> +struct out_flex_midn { struct flexn flex_data; int m; }; /* { dg-bogus"variable sized type not at the end of a struct" } */
> +struct outer_flex_midn { struct out_flex_midn out_flex_data; int p; }; /* { dg-bogus"variable sized type not at the end of a struct" } */
> +union flex_union_midn { int a; struct outer_flex_midn b; }; /* { dg-bogus "variable sized type not at the end of a struct" } */
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size
2023-02-24 18:35 [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
` (2 preceding siblings ...)
2023-03-03 0:02 ` [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
@ 2023-03-08 18:45 ` Kees Cook
3 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2023-03-08 18:45 UTC (permalink / raw)
To: Qing Zhao; +Cc: joseph, rguenther, siddhesh, gcc-patches
On Fri, Feb 24, 2023 at 06:35:03PM +0000, Qing Zhao wrote:
> Hi, Joseph and Richard,
>
> Could you please review this patch and let me know whether it’s ready
> for committing into GCC13?
>
> The fix to Bug PR101832 is an important patch for kernel security
> purpose. it's better to be put into GCC13.
Hi!
This series tests well for me. Getting this landed would be very nice
for the Linux kernel. :)
Are there any additional review notes for it, or is it ready to land?
Thanks!
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-02-24 18:35 ` [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] Qing Zhao
2023-03-03 0:03 ` Qing Zhao
@ 2023-03-09 12:20 ` Richard Biener
2023-03-09 16:11 ` Qing Zhao
1 sibling, 1 reply; 18+ messages in thread
From: Richard Biener @ 2023-03-09 12:20 UTC (permalink / raw)
To: Qing Zhao; +Cc: joseph, siddhesh, keescook, gcc-patches
On Fri, 24 Feb 2023, Qing Zhao wrote:
> GCC extension accepts the case when a struct with a C99 flexible array member
> is embedded into another struct or union (possibly recursively).
> __builtin_object_size should treat such struct as flexible size.
>
> gcc/c/ChangeLog:
>
> PR tree-optimization/101832
> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
> struct/union type.
I can't really comment on the correctness of this part but since
only the C frontend will ever set this and you are using it from
addr_object_size which is also used for other C family languages
(at least), I wonder if you can really test
+ if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
there.
Originally I was suggesting to set this flag in stor-layout.cc
which eventually all languages funnel their types through and
if there's language specific handling use a langhook (with the
default implementation preserving the status quo).
Some more comments below ...
> gcc/cp/ChangeLog:
>
> PR tree-optimization/101832
> * module.cc (trees_out::core_bools): Stream out new bit
> type_include_flexarray.
> (trees_in::core_bools): Stream in new bit type_include_flexarray.
>
> gcc/ChangeLog:
>
> PR tree-optimization/101832
> * print-tree.cc (print_node): Print new bit type_include_flexarray.
> * tree-core.h (struct tree_type_common): New bit
> type_include_flexarray.
> * tree-object-size.cc (addr_object_size): Handle structure/union type
> when it has flexible size.
> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
> in new bit type_include_flexarray.
> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
> out new bit type_include_flexarray.
> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
> TYPE_INCLUDE_FLEXARRAY.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/101832
> * gcc.dg/builtin-object-size-pr101832.c: New test.
> ---
> gcc/c/c-decl.cc | 12 ++
> gcc/cp/module.cc | 2 +
> gcc/print-tree.cc | 5 +
> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
> gcc/tree-core.h | 4 +-
> gcc/tree-object-size.cc | 79 +++++++----
> gcc/tree-streamer-in.cc | 1 +
> gcc/tree-streamer-out.cc | 1 +
> gcc/tree.h | 6 +
> 9 files changed, 215 insertions(+), 29 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>
> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> index 08078eadeb8..f589a2f5192 100644
> --- a/gcc/c/c-decl.cc
> +++ b/gcc/c/c-decl.cc
> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
>
> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> + * when x is an array. */
> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> + when x is the last field. */
> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
> + && is_last_field)
> + TYPE_INCLUDE_FLEXARRAY (t) = true;
> +
> if (DECL_NAME (x)
> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> saw_named_field = true;
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index ac2fe66b080..c750361b704 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
> WB (t->type_common.lang_flag_5);
> WB (t->type_common.lang_flag_6);
> WB (t->type_common.typeless_storage);
> + WB (t->type_common.type_include_flexarray);
> }
>
> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
> RB (t->type_common.lang_flag_5);
> RB (t->type_common.lang_flag_6);
> RB (t->type_common.typeless_storage);
> + RB (t->type_common.type_include_flexarray);
> }
>
> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
> index 1f3afcbbc86..efacdb7686f 100644
> --- a/gcc/print-tree.cc
> +++ b/gcc/print-tree.cc
> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
> && TYPE_CXX_ODR_P (node))
> fputs (" cxx-odr-p", file);
>
> + if ((code == RECORD_TYPE
> + || code == UNION_TYPE)
> + && TYPE_INCLUDE_FLEXARRAY (node))
> + fputs (" include-flexarray", file);
> +
> /* The transparent-union flag is used for different things in
> different nodes. */
> if ((code == UNION_TYPE || code == RECORD_TYPE)
> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> new file mode 100644
> index 00000000000..60078e11634
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> @@ -0,0 +1,134 @@
> +/* PR 101832:
> + GCC extension accepts the case when a struct with a C99 flexible array
> + member is embedded into another struct (possibly recursively).
> + __builtin_object_size will treat such struct as flexible size.
> + However, when a structure with non-C99 flexible array member, i.e, trailing
> + [0], [1], or [4], is embedded into anther struct, the stucture will not
> + be treated as flexible size. */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +#include "builtin-object-size-common.h"
> +
> +#define expect(p, _v) do { \
> + size_t v = _v; \
> + if (p == v) \
> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
> + else {\
> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
> + FAIL (); \
> + } \
> +} while (0);
> +
> +
> +struct A {
> + int n;
> + char data[];
> +};
> +
> +struct B {
> + int m;
> + struct A a;
> +};
> +
> +struct C {
> + int q;
> + struct B b;
> +};
> +
> +struct A0 {
> + int n;
> + char data[0];
> +};
> +
> +struct B0 {
> + int m;
> + struct A0 a;
> +};
> +
> +struct C0 {
> + int q;
> + struct B0 b;
> +};
> +
> +struct A1 {
> + int n;
> + char data[1];
> +};
> +
> +struct B1 {
> + int m;
> + struct A1 a;
> +};
> +
> +struct C1 {
> + int q;
> + struct B1 b;
> +};
> +
> +struct An {
> + int n;
> + char data[8];
> +};
> +
> +struct Bn {
> + int m;
> + struct An a;
> +};
> +
> +struct Cn {
> + int q;
> + struct Bn b;
> +};
> +
> +volatile void *magic1, *magic2;
> +
> +int main (int argc, char *argv[])
> +{
> + struct B *outer;
> + struct C *outest;
> +
> + /* Make sure optimization can't find some other object size. */
> + outer = (void *)magic1;
> + outest = (void *)magic2;
> +
> + expect (__builtin_object_size (&outer->a, 1), -1);
> + expect (__builtin_object_size (&outest->b, 1), -1);
> + expect (__builtin_object_size (&outest->b.a, 1), -1);
> +
> + struct B0 *outer0;
> + struct C0 *outest0;
> +
> + /* Make sure optimization can't find some other object size. */
> + outer0 = (void *)magic1;
> + outest0 = (void *)magic2;
> +
> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
> +
> + struct B1 *outer1;
> + struct C1 *outest1;
> +
> + /* Make sure optimization can't find some other object size. */
> + outer1 = (void *)magic1;
> + outest1 = (void *)magic2;
> +
> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
> +
> + struct Bn *outern;
> + struct Cn *outestn;
> +
> + /* Make sure optimization can't find some other object size. */
> + outern = (void *)magic1;
> + outestn = (void *)magic2;
> +
> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
> +
> + DONE ();
> + return 0;
> +}
> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index acd8deea34e..705d5702b9c 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
> unsigned empty_flag : 1;
> unsigned indivisible_p : 1;
> unsigned no_named_args_stdarg_p : 1;
> - unsigned spare : 15;
> + /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
> + unsigned int type_include_flexarray : 1;
> + unsigned spare : 14;
it looks like the bit could be eventually shared with
no_named_args_stdarg_p (but its accessor doesn't use
FUNC_OR_METHOD_CHECK)
> alias_set_type alias_set;
> tree pointer_to;
> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> index 9a936a91983..22b3c72ea6e 100644
> --- a/gcc/tree-object-size.cc
> +++ b/gcc/tree-object-size.cc
> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
> v = NULL_TREE;
> break;
> case COMPONENT_REF:
> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> + /* When the ref is not to an array, a record or a union, it
> + will not have flexible size, compute the object size
> + directly. */
> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
> {
> v = NULL_TREE;
> break;
> }
> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != UNION_TYPE
> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != QUAL_UNION_TYPE)
> - break;
> - else
> - v = TREE_OPERAND (v, 0);
> - if (TREE_CODE (v) == COMPONENT_REF
> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - == RECORD_TYPE)
> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
> + /* if the record or union does not include a flexible array
> + recursively, compute the object size directly. */
> {
> - /* compute object size only if v is not a
> - flexible array member. */
> - if (!is_flexible_array_mem_ref)
> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> {
> v = NULL_TREE;
> break;
> }
> - v = TREE_OPERAND (v, 0);
> + else
> + v = TREE_OPERAND (v, 0);
> }
> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != UNION_TYPE
> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> - != QUAL_UNION_TYPE)
> - break;
> - else
> - v = TREE_OPERAND (v, 0);
> - if (v != pt_var)
> - v = NULL_TREE;
> else
> - v = pt_var;
> + {
> + /* Now the ref is to an array type. */
> + is_flexible_array_mem_ref
> + = array_ref_flexible_size_p (v);
> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != UNION_TYPE
> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != QUAL_UNION_TYPE)
> + break;
> + else
> + v = TREE_OPERAND (v, 0);
> + if (TREE_CODE (v) == COMPONENT_REF
> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + == RECORD_TYPE)
> + {
> + /* compute object size only if v is not a
> + flexible array member. */
> + if (!is_flexible_array_mem_ref)
> + {
> + v = NULL_TREE;
> + break;
> + }
> + v = TREE_OPERAND (v, 0);
> + }
> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != UNION_TYPE
> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> + != QUAL_UNION_TYPE)
> + break;
> + else
> + v = TREE_OPERAND (v, 0);
> + if (v != pt_var)
> + v = NULL_TREE;
> + else
> + v = pt_var;
> + }
> break;
> default:
> v = pt_var;
> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
> index d4dc30f048f..c19ede0631d 100644
> --- a/gcc/tree-streamer-in.cc
> +++ b/gcc/tree-streamer-in.cc
> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
> }
> else if (TREE_CODE (expr) == ARRAY_TYPE)
> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
> index d107229da5c..73e4b4e547c 100644
> --- a/gcc/tree-streamer-out.cc
> +++ b/gcc/tree-streamer-out.cc
> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
> : TYPE_CXX_ODR_P (expr), 1);
> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
> }
> else if (TREE_CODE (expr) == ARRAY_TYPE)
> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
> diff --git a/gcc/tree.h b/gcc/tree.h
> index 92ac0e6a214..ab1cdc3dc85 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>
> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> + at the last field recursively. */
> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
Please use RECORD_OR_UNION_CHECK. The comment "at the last field"
doesn't seem to match implementation? (at least not obviously)
Given this is a generic header expanding on what a "flexible array member"
is to the middle-end here would be good. Especially the guarantees
made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
vs. DECL_FLEXARRAY).
Thanks,
Richard.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-03-09 12:20 ` Richard Biener
@ 2023-03-09 16:11 ` Qing Zhao
2023-03-10 7:54 ` Richard Biener
0 siblings, 1 reply; 18+ messages in thread
From: Qing Zhao @ 2023-03-09 16:11 UTC (permalink / raw)
To: Richard Biener; +Cc: joseph, siddhesh, keescook, gcc-patches
> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@suse.de> wrote:
>
> On Fri, 24 Feb 2023, Qing Zhao wrote:
>
>> GCC extension accepts the case when a struct with a C99 flexible array member
>> is embedded into another struct or union (possibly recursively).
>> __builtin_object_size should treat such struct as flexible size.
>>
>> gcc/c/ChangeLog:
>>
>> PR tree-optimization/101832
>> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
>> struct/union type.
>
> I can't really comment on the correctness of this part but since
> only the C frontend will ever set this and you are using it from
> addr_object_size which is also used for other C family languages
> (at least), I wonder if you can really test
>
> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>
> there.
You mean for C++ and also other C family languages (other than C), the above bit cannot be set?
Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY, which is only set for C too.
So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set TYPE_INCLUDE_FLEXARRAY also in C++ FE?
What’s your suggestion?
(I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be set in the same places).
>
> Originally I was suggesting to set this flag in stor-layout.cc
> which eventually all languages funnel their types through and
> if there's language specific handling use a langhook (with the
> default implementation preserving the status quo).
If we decide to set the bits in stor-layout.cc, where is the best place to do it? I checked the star-layout.cc code, looks like “layout_type” might be the place where we can set these bits for RECORD_TYPE, UNION_TYPE?
>
> Some more comments below ...
>
>> gcc/cp/ChangeLog:
>>
>> PR tree-optimization/101832
>> * module.cc (trees_out::core_bools): Stream out new bit
>> type_include_flexarray.
>> (trees_in::core_bools): Stream in new bit type_include_flexarray.
>>
>> gcc/ChangeLog:
>>
>> PR tree-optimization/101832
>> * print-tree.cc (print_node): Print new bit type_include_flexarray.
>> * tree-core.h (struct tree_type_common): New bit
>> type_include_flexarray.
>> * tree-object-size.cc (addr_object_size): Handle structure/union type
>> when it has flexible size.
>> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
>> in new bit type_include_flexarray.
>> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
>> out new bit type_include_flexarray.
>> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
>> TYPE_INCLUDE_FLEXARRAY.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR tree-optimization/101832
>> * gcc.dg/builtin-object-size-pr101832.c: New test.
>> ---
>> gcc/c/c-decl.cc | 12 ++
>> gcc/cp/module.cc | 2 +
>> gcc/print-tree.cc | 5 +
>> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
>> gcc/tree-core.h | 4 +-
>> gcc/tree-object-size.cc | 79 +++++++----
>> gcc/tree-streamer-in.cc | 1 +
>> gcc/tree-streamer-out.cc | 1 +
>> gcc/tree.h | 6 +
>> 9 files changed, 215 insertions(+), 29 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>
>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
>> index 08078eadeb8..f589a2f5192 100644
>> --- a/gcc/c/c-decl.cc
>> +++ b/gcc/c/c-decl.cc
>> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
>> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
>> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
>>
>> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>> + * when x is an array. */
>> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
>> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
>> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>> + when x is the last field. */
>> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
>> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
>> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
>> + && is_last_field)
>> + TYPE_INCLUDE_FLEXARRAY (t) = true;
>> +
>> if (DECL_NAME (x)
>> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
>> saw_named_field = true;
>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>> index ac2fe66b080..c750361b704 100644
>> --- a/gcc/cp/module.cc
>> +++ b/gcc/cp/module.cc
>> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
>> WB (t->type_common.lang_flag_5);
>> WB (t->type_common.lang_flag_6);
>> WB (t->type_common.typeless_storage);
>> + WB (t->type_common.type_include_flexarray);
>> }
>>
>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
>> RB (t->type_common.lang_flag_5);
>> RB (t->type_common.lang_flag_6);
>> RB (t->type_common.typeless_storage);
>> + RB (t->type_common.type_include_flexarray);
>> }
>>
>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
>> index 1f3afcbbc86..efacdb7686f 100644
>> --- a/gcc/print-tree.cc
>> +++ b/gcc/print-tree.cc
>> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
>> && TYPE_CXX_ODR_P (node))
>> fputs (" cxx-odr-p", file);
>>
>> + if ((code == RECORD_TYPE
>> + || code == UNION_TYPE)
>> + && TYPE_INCLUDE_FLEXARRAY (node))
>> + fputs (" include-flexarray", file);
>> +
>> /* The transparent-union flag is used for different things in
>> different nodes. */
>> if ((code == UNION_TYPE || code == RECORD_TYPE)
>> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>> new file mode 100644
>> index 00000000000..60078e11634
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>> @@ -0,0 +1,134 @@
>> +/* PR 101832:
>> + GCC extension accepts the case when a struct with a C99 flexible array
>> + member is embedded into another struct (possibly recursively).
>> + __builtin_object_size will treat such struct as flexible size.
>> + However, when a structure with non-C99 flexible array member, i.e, trailing
>> + [0], [1], or [4], is embedded into anther struct, the stucture will not
>> + be treated as flexible size. */
>> +/* { dg-do run } */
>> +/* { dg-options "-O2" } */
>> +
>> +#include "builtin-object-size-common.h"
>> +
>> +#define expect(p, _v) do { \
>> + size_t v = _v; \
>> + if (p == v) \
>> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
>> + else {\
>> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
>> + FAIL (); \
>> + } \
>> +} while (0);
>> +
>> +
>> +struct A {
>> + int n;
>> + char data[];
>> +};
>> +
>> +struct B {
>> + int m;
>> + struct A a;
>> +};
>> +
>> +struct C {
>> + int q;
>> + struct B b;
>> +};
>> +
>> +struct A0 {
>> + int n;
>> + char data[0];
>> +};
>> +
>> +struct B0 {
>> + int m;
>> + struct A0 a;
>> +};
>> +
>> +struct C0 {
>> + int q;
>> + struct B0 b;
>> +};
>> +
>> +struct A1 {
>> + int n;
>> + char data[1];
>> +};
>> +
>> +struct B1 {
>> + int m;
>> + struct A1 a;
>> +};
>> +
>> +struct C1 {
>> + int q;
>> + struct B1 b;
>> +};
>> +
>> +struct An {
>> + int n;
>> + char data[8];
>> +};
>> +
>> +struct Bn {
>> + int m;
>> + struct An a;
>> +};
>> +
>> +struct Cn {
>> + int q;
>> + struct Bn b;
>> +};
>> +
>> +volatile void *magic1, *magic2;
>> +
>> +int main (int argc, char *argv[])
>> +{
>> + struct B *outer;
>> + struct C *outest;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outer = (void *)magic1;
>> + outest = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outer->a, 1), -1);
>> + expect (__builtin_object_size (&outest->b, 1), -1);
>> + expect (__builtin_object_size (&outest->b.a, 1), -1);
>> +
>> + struct B0 *outer0;
>> + struct C0 *outest0;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outer0 = (void *)magic1;
>> + outest0 = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
>> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
>> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
>> +
>> + struct B1 *outer1;
>> + struct C1 *outest1;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outer1 = (void *)magic1;
>> + outest1 = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
>> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
>> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
>> +
>> + struct Bn *outern;
>> + struct Cn *outestn;
>> +
>> + /* Make sure optimization can't find some other object size. */
>> + outern = (void *)magic1;
>> + outestn = (void *)magic2;
>> +
>> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
>> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
>> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
>> +
>> + DONE ();
>> + return 0;
>> +}
>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>> index acd8deea34e..705d5702b9c 100644
>> --- a/gcc/tree-core.h
>> +++ b/gcc/tree-core.h
>> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
>> unsigned empty_flag : 1;
>> unsigned indivisible_p : 1;
>> RECORD_OR_UNION_CHECK
>
> it looks like the bit could be eventually shared with
> no_named_args_stdarg_p (but its accessor doesn't use
> FUNC_OR_METHOD_CHECK)
You mean to let “type_include_flexarray” share the same bit with “no_named_args_stdarg_p” in order to save 1-bit space in IR?
Then change the
/* True if this is a stdarg function with no named arguments (C2x
(...) prototype, where arguments can be accessed with va_start and
va_arg), as opposed to an unprototyped function. */
#define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
(TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
at the last field recursively. */
#define TYPE_INCLUDE_FLEXARRAY(NODE) \
(TYPE_CHECK (NODE)->type_common.type_include_flexarray)
To:
union {
unsigned no_named_args_stdarg_p : 1;
/* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
unsigned int type_include_flexarray : 1;
} shared_bit;
unsigned spare : 15;
/* True if this is a stdarg function with no named arguments (C2x
(...) prototype, where arguments can be accessed with va_start and
va_arg), as opposed to an unprototyped function. */
#define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
(FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
at the last field recursively. */
#define TYPE_INCLUDE_FLEXARRAY(NODE) \
(RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)
>
>> alias_set_type alias_set;
>> tree pointer_to;
>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
>> index 9a936a91983..22b3c72ea6e 100644
>> --- a/gcc/tree-object-size.cc
>> +++ b/gcc/tree-object-size.cc
>> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
>> v = NULL_TREE;
>> break;
>> case COMPONENT_REF:
>> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>> + /* When the ref is not to an array, a record or a union, it
>> + will not have flexible size, compute the object size
>> + directly. */
>> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
>> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
>> {
>> v = NULL_TREE;
>> break;
>> }
>> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != UNION_TYPE
>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != QUAL_UNION_TYPE)
>> - break;
>> - else
>> - v = TREE_OPERAND (v, 0);
>> - if (TREE_CODE (v) == COMPONENT_REF
>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - == RECORD_TYPE)
>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
>> + /* if the record or union does not include a flexible array
>> + recursively, compute the object size directly. */
>> {
>> - /* compute object size only if v is not a
>> - flexible array member. */
>> - if (!is_flexible_array_mem_ref)
>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>> {
>> v = NULL_TREE;
>> break;
>> }
>> - v = TREE_OPERAND (v, 0);
>> + else
>> + v = TREE_OPERAND (v, 0);
>> }
>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != UNION_TYPE
>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> - != QUAL_UNION_TYPE)
>> - break;
>> - else
>> - v = TREE_OPERAND (v, 0);
>> - if (v != pt_var)
>> - v = NULL_TREE;
>> else
>> - v = pt_var;
>> + {
>> + /* Now the ref is to an array type. */
>> + is_flexible_array_mem_ref
>> + = array_ref_flexible_size_p (v);
>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != UNION_TYPE
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != QUAL_UNION_TYPE)
>> + break;
>> + else
>> + v = TREE_OPERAND (v, 0);
>> + if (TREE_CODE (v) == COMPONENT_REF
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + == RECORD_TYPE)
>> + {
>> + /* compute object size only if v is not a
>> + flexible array member. */
>> + if (!is_flexible_array_mem_ref)
>> + {
>> + v = NULL_TREE;
>> + break;
>> + }
>> + v = TREE_OPERAND (v, 0);
>> + }
>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != UNION_TYPE
>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>> + != QUAL_UNION_TYPE)
>> + break;
>> + else
>> + v = TREE_OPERAND (v, 0);
>> + if (v != pt_var)
>> + v = NULL_TREE;
>> + else
>> + v = pt_var;
>> + }
>> break;
>> default:
>> v = pt_var;
>> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
>> index d4dc30f048f..c19ede0631d 100644
>> --- a/gcc/tree-streamer-in.cc
>> +++ b/gcc/tree-streamer-in.cc
>> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
>> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
>> }
>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
>> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
>> index d107229da5c..73e4b4e547c 100644
>> --- a/gcc/tree-streamer-out.cc
>> +++ b/gcc/tree-streamer-out.cc
>> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
>> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
>> : TYPE_CXX_ODR_P (expr), 1);
>> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
>> }
>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
>> diff --git a/gcc/tree.h b/gcc/tree.h
>> index 92ac0e6a214..ab1cdc3dc85 100644
>> --- a/gcc/tree.h
>> +++ b/gcc/tree.h
>> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>
>> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>> + at the last field recursively. */
>> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
>> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>
> Please use RECORD_OR_UNION_CHECK.
Okay.
> The comment "at the last field"
> doesn't seem to match implementation? (at least not obviously)
The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the last field. (And recursively).
> Given this is a generic header expanding on what a "flexible array member"
> is to the middle-end here would be good. Especially the guarantees
> made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
> vs. DECL_FLEXARRAY).
The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex array by default.
It’s only set to TRUE when the struct/union includes a flex array at the last field (and recursively). So, I think it should be correct.
Let me know if I missed anything here.
Thanks a lot for your comments.
Qing
>
> Thanks,
> Richard.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-03-09 16:11 ` Qing Zhao
@ 2023-03-10 7:54 ` Richard Biener
2023-03-10 13:48 ` Qing Zhao
0 siblings, 1 reply; 18+ messages in thread
From: Richard Biener @ 2023-03-10 7:54 UTC (permalink / raw)
To: Qing Zhao; +Cc: joseph, siddhesh, keescook, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 19308 bytes --]
On Thu, 9 Mar 2023, Qing Zhao wrote:
>
>
> > On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@suse.de> wrote:
> >
> > On Fri, 24 Feb 2023, Qing Zhao wrote:
> >
> >> GCC extension accepts the case when a struct with a C99 flexible array member
> >> is embedded into another struct or union (possibly recursively).
> >> __builtin_object_size should treat such struct as flexible size.
> >>
> >> gcc/c/ChangeLog:
> >>
> >> PR tree-optimization/101832
> >> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
> >> struct/union type.
> >
> > I can't really comment on the correctness of this part but since
> > only the C frontend will ever set this and you are using it from
> > addr_object_size which is also used for other C family languages
> > (at least), I wonder if you can really test
> >
> > + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> >
> > there.
>
> You mean for C++ and also other C family languages (other than C), the above bit cannot be set?
> Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY, which is only set for C too.
> So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set TYPE_INCLUDE_FLEXARRAY also in C++ FE?
> What’s your suggestion?
>
> (I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be set in the same places).
I was wondering if the above test errors on the conservative side
correctly - it will now, for all but C, cut off some thing where it
didn't before?
> >
> > Originally I was suggesting to set this flag in stor-layout.cc
> > which eventually all languages funnel their types through and
> > if there's language specific handling use a langhook (with the
> > default implementation preserving the status quo).
>
> If we decide to set the bits in stor-layout.cc, where is the best place to do it? I checked the star-layout.cc code, looks like “layout_type” might be the place where we can set these bits for RECORD_TYPE, UNION_TYPE?
Yes, it would be layout_type.
> >
> > Some more comments below ...
> >
> >> gcc/cp/ChangeLog:
> >>
> >> PR tree-optimization/101832
> >> * module.cc (trees_out::core_bools): Stream out new bit
> >> type_include_flexarray.
> >> (trees_in::core_bools): Stream in new bit type_include_flexarray.
> >>
> >> gcc/ChangeLog:
> >>
> >> PR tree-optimization/101832
> >> * print-tree.cc (print_node): Print new bit type_include_flexarray.
> >> * tree-core.h (struct tree_type_common): New bit
> >> type_include_flexarray.
> >> * tree-object-size.cc (addr_object_size): Handle structure/union type
> >> when it has flexible size.
> >> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
> >> in new bit type_include_flexarray.
> >> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
> >> out new bit type_include_flexarray.
> >> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
> >> TYPE_INCLUDE_FLEXARRAY.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> PR tree-optimization/101832
> >> * gcc.dg/builtin-object-size-pr101832.c: New test.
> >> ---
> >> gcc/c/c-decl.cc | 12 ++
> >> gcc/cp/module.cc | 2 +
> >> gcc/print-tree.cc | 5 +
> >> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
> >> gcc/tree-core.h | 4 +-
> >> gcc/tree-object-size.cc | 79 +++++++----
> >> gcc/tree-streamer-in.cc | 1 +
> >> gcc/tree-streamer-out.cc | 1 +
> >> gcc/tree.h | 6 +
> >> 9 files changed, 215 insertions(+), 29 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >>
> >> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> >> index 08078eadeb8..f589a2f5192 100644
> >> --- a/gcc/c/c-decl.cc
> >> +++ b/gcc/c/c-decl.cc
> >> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> >> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
> >> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
> >>
> >> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> >> + * when x is an array. */
> >> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
> >> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
> >> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> >> + when x is the last field. */
> >> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
> >> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
> >> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
> >> + && is_last_field)
> >> + TYPE_INCLUDE_FLEXARRAY (t) = true;
> >> +
> >> if (DECL_NAME (x)
> >> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> >> saw_named_field = true;
> >> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> >> index ac2fe66b080..c750361b704 100644
> >> --- a/gcc/cp/module.cc
> >> +++ b/gcc/cp/module.cc
> >> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
> >> WB (t->type_common.lang_flag_5);
> >> WB (t->type_common.lang_flag_6);
> >> WB (t->type_common.typeless_storage);
> >> + WB (t->type_common.type_include_flexarray);
> >> }
> >>
> >> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> >> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
> >> RB (t->type_common.lang_flag_5);
> >> RB (t->type_common.lang_flag_6);
> >> RB (t->type_common.typeless_storage);
> >> + RB (t->type_common.type_include_flexarray);
> >> }
> >>
> >> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> >> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
> >> index 1f3afcbbc86..efacdb7686f 100644
> >> --- a/gcc/print-tree.cc
> >> +++ b/gcc/print-tree.cc
> >> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
> >> && TYPE_CXX_ODR_P (node))
> >> fputs (" cxx-odr-p", file);
> >>
> >> + if ((code == RECORD_TYPE
> >> + || code == UNION_TYPE)
> >> + && TYPE_INCLUDE_FLEXARRAY (node))
> >> + fputs (" include-flexarray", file);
> >> +
> >> /* The transparent-union flag is used for different things in
> >> different nodes. */
> >> if ((code == UNION_TYPE || code == RECORD_TYPE)
> >> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >> new file mode 100644
> >> index 00000000000..60078e11634
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >> @@ -0,0 +1,134 @@
> >> +/* PR 101832:
> >> + GCC extension accepts the case when a struct with a C99 flexible array
> >> + member is embedded into another struct (possibly recursively).
> >> + __builtin_object_size will treat such struct as flexible size.
> >> + However, when a structure with non-C99 flexible array member, i.e, trailing
> >> + [0], [1], or [4], is embedded into anther struct, the stucture will not
> >> + be treated as flexible size. */
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O2" } */
> >> +
> >> +#include "builtin-object-size-common.h"
> >> +
> >> +#define expect(p, _v) do { \
> >> + size_t v = _v; \
> >> + if (p == v) \
> >> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
> >> + else {\
> >> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
> >> + FAIL (); \
> >> + } \
> >> +} while (0);
> >> +
> >> +
> >> +struct A {
> >> + int n;
> >> + char data[];
> >> +};
> >> +
> >> +struct B {
> >> + int m;
> >> + struct A a;
> >> +};
> >> +
> >> +struct C {
> >> + int q;
> >> + struct B b;
> >> +};
> >> +
> >> +struct A0 {
> >> + int n;
> >> + char data[0];
> >> +};
> >> +
> >> +struct B0 {
> >> + int m;
> >> + struct A0 a;
> >> +};
> >> +
> >> +struct C0 {
> >> + int q;
> >> + struct B0 b;
> >> +};
> >> +
> >> +struct A1 {
> >> + int n;
> >> + char data[1];
> >> +};
> >> +
> >> +struct B1 {
> >> + int m;
> >> + struct A1 a;
> >> +};
> >> +
> >> +struct C1 {
> >> + int q;
> >> + struct B1 b;
> >> +};
> >> +
> >> +struct An {
> >> + int n;
> >> + char data[8];
> >> +};
> >> +
> >> +struct Bn {
> >> + int m;
> >> + struct An a;
> >> +};
> >> +
> >> +struct Cn {
> >> + int q;
> >> + struct Bn b;
> >> +};
> >> +
> >> +volatile void *magic1, *magic2;
> >> +
> >> +int main (int argc, char *argv[])
> >> +{
> >> + struct B *outer;
> >> + struct C *outest;
> >> +
> >> + /* Make sure optimization can't find some other object size. */
> >> + outer = (void *)magic1;
> >> + outest = (void *)magic2;
> >> +
> >> + expect (__builtin_object_size (&outer->a, 1), -1);
> >> + expect (__builtin_object_size (&outest->b, 1), -1);
> >> + expect (__builtin_object_size (&outest->b.a, 1), -1);
> >> +
> >> + struct B0 *outer0;
> >> + struct C0 *outest0;
> >> +
> >> + /* Make sure optimization can't find some other object size. */
> >> + outer0 = (void *)magic1;
> >> + outest0 = (void *)magic2;
> >> +
> >> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
> >> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
> >> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
> >> +
> >> + struct B1 *outer1;
> >> + struct C1 *outest1;
> >> +
> >> + /* Make sure optimization can't find some other object size. */
> >> + outer1 = (void *)magic1;
> >> + outest1 = (void *)magic2;
> >> +
> >> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
> >> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
> >> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
> >> +
> >> + struct Bn *outern;
> >> + struct Cn *outestn;
> >> +
> >> + /* Make sure optimization can't find some other object size. */
> >> + outern = (void *)magic1;
> >> + outestn = (void *)magic2;
> >> +
> >> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
> >> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
> >> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
> >> +
> >> + DONE ();
> >> + return 0;
> >> +}
> >> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> >> index acd8deea34e..705d5702b9c 100644
> >> --- a/gcc/tree-core.h
> >> +++ b/gcc/tree-core.h
> >> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
> >> unsigned empty_flag : 1;
> >> unsigned indivisible_p : 1;
> >> RECORD_OR_UNION_CHECK
> >
> > it looks like the bit could be eventually shared with
> > no_named_args_stdarg_p (but its accessor doesn't use
> > FUNC_OR_METHOD_CHECK)
> You mean to let “type_include_flexarray” share the same bit with “no_named_args_stdarg_p” in order to save 1-bit space in IR?
> Then change the
>
> /* True if this is a stdarg function with no named arguments (C2x
> (...) prototype, where arguments can be accessed with va_start and
> va_arg), as opposed to an unprototyped function. */
> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>
> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> at the last field recursively. */
> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
> (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>
> To:
>
> union {
> unsigned no_named_args_stdarg_p : 1;
> /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
> unsigned int type_include_flexarray : 1;
> } shared_bit;
> unsigned spare : 15;
>
> /* True if this is a stdarg function with no named arguments (C2x
> (...) prototype, where arguments can be accessed with va_start and
> va_arg), as opposed to an unprototyped function. */
> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> (FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
>
> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> at the last field recursively. */
> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
> (RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)
No, we're just overloading bits by using the same name for different
purposes. I don't think such a union would pack nicely. We overload
by documenting the uses, in the above cases the uses are separated
by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE
and the accessor macros should be updated to use the appropriate
tree check macros covering that so we don't try to inspect the
"wrong bit".
> >
> >> alias_set_type alias_set;
> >> tree pointer_to;
> >> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> >> index 9a936a91983..22b3c72ea6e 100644
> >> --- a/gcc/tree-object-size.cc
> >> +++ b/gcc/tree-object-size.cc
> >> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
> >> v = NULL_TREE;
> >> break;
> >> case COMPONENT_REF:
> >> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> >> + /* When the ref is not to an array, a record or a union, it
> >> + will not have flexible size, compute the object size
> >> + directly. */
> >> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> >> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
> >> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
> >> {
> >> v = NULL_TREE;
> >> break;
> >> }
> >> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
> >> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> - != UNION_TYPE
> >> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> - != QUAL_UNION_TYPE)
> >> - break;
> >> - else
> >> - v = TREE_OPERAND (v, 0);
> >> - if (TREE_CODE (v) == COMPONENT_REF
> >> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> - == RECORD_TYPE)
> >> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
> >> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
> >> + /* if the record or union does not include a flexible array
> >> + recursively, compute the object size directly. */
> >> {
> >> - /* compute object size only if v is not a
> >> - flexible array member. */
> >> - if (!is_flexible_array_mem_ref)
> >> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> >> {
> >> v = NULL_TREE;
> >> break;
> >> }
> >> - v = TREE_OPERAND (v, 0);
> >> + else
> >> + v = TREE_OPERAND (v, 0);
> >> }
> >> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> - != UNION_TYPE
> >> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> - != QUAL_UNION_TYPE)
> >> - break;
> >> - else
> >> - v = TREE_OPERAND (v, 0);
> >> - if (v != pt_var)
> >> - v = NULL_TREE;
> >> else
> >> - v = pt_var;
> >> + {
> >> + /* Now the ref is to an array type. */
> >> + is_flexible_array_mem_ref
> >> + = array_ref_flexible_size_p (v);
> >> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> + != UNION_TYPE
> >> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> + != QUAL_UNION_TYPE)
> >> + break;
> >> + else
> >> + v = TREE_OPERAND (v, 0);
> >> + if (TREE_CODE (v) == COMPONENT_REF
> >> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> + == RECORD_TYPE)
> >> + {
> >> + /* compute object size only if v is not a
> >> + flexible array member. */
> >> + if (!is_flexible_array_mem_ref)
> >> + {
> >> + v = NULL_TREE;
> >> + break;
> >> + }
> >> + v = TREE_OPERAND (v, 0);
> >> + }
> >> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> + != UNION_TYPE
> >> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >> + != QUAL_UNION_TYPE)
> >> + break;
> >> + else
> >> + v = TREE_OPERAND (v, 0);
> >> + if (v != pt_var)
> >> + v = NULL_TREE;
> >> + else
> >> + v = pt_var;
> >> + }
> >> break;
> >> default:
> >> v = pt_var;
> >> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
> >> index d4dc30f048f..c19ede0631d 100644
> >> --- a/gcc/tree-streamer-in.cc
> >> +++ b/gcc/tree-streamer-in.cc
> >> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> >> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
> >> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> >> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> >> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
> >> }
> >> else if (TREE_CODE (expr) == ARRAY_TYPE)
> >> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
> >> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
> >> index d107229da5c..73e4b4e547c 100644
> >> --- a/gcc/tree-streamer-out.cc
> >> +++ b/gcc/tree-streamer-out.cc
> >> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> >> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
> >> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
> >> : TYPE_CXX_ODR_P (expr), 1);
> >> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
> >> }
> >> else if (TREE_CODE (expr) == ARRAY_TYPE)
> >> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
> >> diff --git a/gcc/tree.h b/gcc/tree.h
> >> index 92ac0e6a214..ab1cdc3dc85 100644
> >> --- a/gcc/tree.h
> >> +++ b/gcc/tree.h
> >> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
> >> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> >> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> >>
> >> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> >> + at the last field recursively. */
> >> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
> >> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
> >
> > Please use RECORD_OR_UNION_CHECK.
> Okay.
> > The comment "at the last field"
> > doesn't seem to match implementation? (at least not obviously)
> The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the last field. (And recursively).
> > Given this is a generic header expanding on what a "flexible array member"
> > is to the middle-end here would be good. Especially the guarantees
> > made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
> > vs. DECL_FLEXARRAY).
>
> The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex array by default.
> It’s only set to TRUE when the struct/union includes a flex array at the last field (and recursively). So, I think it should be correct.
> Let me know if I missed anything here.
See above - I was looking at the use (but I'm not familiar with that
code), and wondered how the change affects uses from other frontends.
Richard.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-03-10 7:54 ` Richard Biener
@ 2023-03-10 13:48 ` Qing Zhao
2023-03-13 22:42 ` Qing Zhao
0 siblings, 1 reply; 18+ messages in thread
From: Qing Zhao @ 2023-03-10 13:48 UTC (permalink / raw)
To: Richard Biener; +Cc: joseph, siddhesh, keescook, gcc-patches
> On Mar 10, 2023, at 2:54 AM, Richard Biener <rguenther@suse.de> wrote:
>
> On Thu, 9 Mar 2023, Qing Zhao wrote:
>
>>
>>
>>> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@suse.de> wrote:
>>>
>>> On Fri, 24 Feb 2023, Qing Zhao wrote:
>>>
>>>> GCC extension accepts the case when a struct with a C99 flexible array member
>>>> is embedded into another struct or union (possibly recursively).
>>>> __builtin_object_size should treat such struct as flexible size.
>>>>
>>>> gcc/c/ChangeLog:
>>>>
>>>> PR tree-optimization/101832
>>>> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
>>>> struct/union type.
>>>
>>> I can't really comment on the correctness of this part but since
>>> only the C frontend will ever set this and you are using it from
>>> addr_object_size which is also used for other C family languages
>>> (at least), I wonder if you can really test
>>>
>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>>>
>>> there.
>>
>> You mean for C++ and also other C family languages (other than C), the above bit cannot be set?
>> Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY, which is only set for C too.
>> So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set TYPE_INCLUDE_FLEXARRAY also in C++ FE?
>> What’s your suggestion?
>>
>> (I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be set in the same places).
>
> I was wondering if the above test errors on the conservative side
> correctly - it will now, for all but C, cut off some thing where it
> didn't before?
As long as the default value of TYPE_INCLUDE_FLEXARRAY reflects the correct conservative behavior, then the testing should be correct, right?
The default value of TYPE_INCLUDE_FLEXARRAY is 0, i.e, FALSE, means that the TYPE does not include a flex array by default, this is the correct behavior. Only when the TYPE does include a flexiarray, it will be set to TRUE. So, I think it’s correct.
This is a different situation for DECL_NOT_FLEXARRAY, by default, the compiler will treat ALL trailing arrays as FMA, so in order to keep the correct conservative behavior, we should keep the default value for DECL_NOT_FLEXARRAY as it’s a FMA, i.e, DECL_NOT_FLEXARRAY being FALSE, by default. Only when the array is NOT a FMA, we set it to true.
So, the default value for TYPE_INCLUDE_FLEXARRAY is correct.
>
>>>
>>> Originally I was suggesting to set this flag in stor-layout.cc
>>> which eventually all languages funnel their types through and
>>> if there's language specific handling use a langhook (with the
>>> default implementation preserving the status quo).
>>
>> If we decide to set the bits in stor-layout.cc, where is the best place to do it? I checked the star-layout.cc code, looks like “layout_type” might be the place where we can set these bits for RECORD_TYPE, UNION_TYPE?
>
> Yes, it would be layout_type.
>
>>>
>>> Some more comments below ...
>>>
>>>> gcc/cp/ChangeLog:
>>>>
>>>> PR tree-optimization/101832
>>>> * module.cc (trees_out::core_bools): Stream out new bit
>>>> type_include_flexarray.
>>>> (trees_in::core_bools): Stream in new bit type_include_flexarray.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> PR tree-optimization/101832
>>>> * print-tree.cc (print_node): Print new bit type_include_flexarray.
>>>> * tree-core.h (struct tree_type_common): New bit
>>>> type_include_flexarray.
>>>> * tree-object-size.cc (addr_object_size): Handle structure/union type
>>>> when it has flexible size.
>>>> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
>>>> in new bit type_include_flexarray.
>>>> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
>>>> out new bit type_include_flexarray.
>>>> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
>>>> TYPE_INCLUDE_FLEXARRAY.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> PR tree-optimization/101832
>>>> * gcc.dg/builtin-object-size-pr101832.c: New test.
>>>> ---
>>>> gcc/c/c-decl.cc | 12 ++
>>>> gcc/cp/module.cc | 2 +
>>>> gcc/print-tree.cc | 5 +
>>>> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
>>>> gcc/tree-core.h | 4 +-
>>>> gcc/tree-object-size.cc | 79 +++++++----
>>>> gcc/tree-streamer-in.cc | 1 +
>>>> gcc/tree-streamer-out.cc | 1 +
>>>> gcc/tree.h | 6 +
>>>> 9 files changed, 215 insertions(+), 29 deletions(-)
>>>> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>>
>>>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
>>>> index 08078eadeb8..f589a2f5192 100644
>>>> --- a/gcc/c/c-decl.cc
>>>> +++ b/gcc/c/c-decl.cc
>>>> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
>>>> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
>>>> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
>>>>
>>>> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>>>> + * when x is an array. */
>>>> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
>>>> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
>>>> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>>>> + when x is the last field. */
>>>> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
>>>> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
>>>> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
>>>> + && is_last_field)
>>>> + TYPE_INCLUDE_FLEXARRAY (t) = true;
>>>> +
>>>> if (DECL_NAME (x)
>>>> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
>>>> saw_named_field = true;
>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>>>> index ac2fe66b080..c750361b704 100644
>>>> --- a/gcc/cp/module.cc
>>>> +++ b/gcc/cp/module.cc
>>>> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
>>>> WB (t->type_common.lang_flag_5);
>>>> WB (t->type_common.lang_flag_6);
>>>> WB (t->type_common.typeless_storage);
>>>> + WB (t->type_common.type_include_flexarray);
>>>> }
>>>>
>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>>>> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
>>>> RB (t->type_common.lang_flag_5);
>>>> RB (t->type_common.lang_flag_6);
>>>> RB (t->type_common.typeless_storage);
>>>> + RB (t->type_common.type_include_flexarray);
>>>> }
>>>>
>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>>>> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
>>>> index 1f3afcbbc86..efacdb7686f 100644
>>>> --- a/gcc/print-tree.cc
>>>> +++ b/gcc/print-tree.cc
>>>> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
>>>> && TYPE_CXX_ODR_P (node))
>>>> fputs (" cxx-odr-p", file);
>>>>
>>>> + if ((code == RECORD_TYPE
>>>> + || code == UNION_TYPE)
>>>> + && TYPE_INCLUDE_FLEXARRAY (node))
>>>> + fputs (" include-flexarray", file);
>>>> +
>>>> /* The transparent-union flag is used for different things in
>>>> different nodes. */
>>>> if ((code == UNION_TYPE || code == RECORD_TYPE)
>>>> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>> new file mode 100644
>>>> index 00000000000..60078e11634
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>> @@ -0,0 +1,134 @@
>>>> +/* PR 101832:
>>>> + GCC extension accepts the case when a struct with a C99 flexible array
>>>> + member is embedded into another struct (possibly recursively).
>>>> + __builtin_object_size will treat such struct as flexible size.
>>>> + However, when a structure with non-C99 flexible array member, i.e, trailing
>>>> + [0], [1], or [4], is embedded into anther struct, the stucture will not
>>>> + be treated as flexible size. */
>>>> +/* { dg-do run } */
>>>> +/* { dg-options "-O2" } */
>>>> +
>>>> +#include "builtin-object-size-common.h"
>>>> +
>>>> +#define expect(p, _v) do { \
>>>> + size_t v = _v; \
>>>> + if (p == v) \
>>>> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
>>>> + else {\
>>>> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
>>>> + FAIL (); \
>>>> + } \
>>>> +} while (0);
>>>> +
>>>> +
>>>> +struct A {
>>>> + int n;
>>>> + char data[];
>>>> +};
>>>> +
>>>> +struct B {
>>>> + int m;
>>>> + struct A a;
>>>> +};
>>>> +
>>>> +struct C {
>>>> + int q;
>>>> + struct B b;
>>>> +};
>>>> +
>>>> +struct A0 {
>>>> + int n;
>>>> + char data[0];
>>>> +};
>>>> +
>>>> +struct B0 {
>>>> + int m;
>>>> + struct A0 a;
>>>> +};
>>>> +
>>>> +struct C0 {
>>>> + int q;
>>>> + struct B0 b;
>>>> +};
>>>> +
>>>> +struct A1 {
>>>> + int n;
>>>> + char data[1];
>>>> +};
>>>> +
>>>> +struct B1 {
>>>> + int m;
>>>> + struct A1 a;
>>>> +};
>>>> +
>>>> +struct C1 {
>>>> + int q;
>>>> + struct B1 b;
>>>> +};
>>>> +
>>>> +struct An {
>>>> + int n;
>>>> + char data[8];
>>>> +};
>>>> +
>>>> +struct Bn {
>>>> + int m;
>>>> + struct An a;
>>>> +};
>>>> +
>>>> +struct Cn {
>>>> + int q;
>>>> + struct Bn b;
>>>> +};
>>>> +
>>>> +volatile void *magic1, *magic2;
>>>> +
>>>> +int main (int argc, char *argv[])
>>>> +{
>>>> + struct B *outer;
>>>> + struct C *outest;
>>>> +
>>>> + /* Make sure optimization can't find some other object size. */
>>>> + outer = (void *)magic1;
>>>> + outest = (void *)magic2;
>>>> +
>>>> + expect (__builtin_object_size (&outer->a, 1), -1);
>>>> + expect (__builtin_object_size (&outest->b, 1), -1);
>>>> + expect (__builtin_object_size (&outest->b.a, 1), -1);
>>>> +
>>>> + struct B0 *outer0;
>>>> + struct C0 *outest0;
>>>> +
>>>> + /* Make sure optimization can't find some other object size. */
>>>> + outer0 = (void *)magic1;
>>>> + outest0 = (void *)magic2;
>>>> +
>>>> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
>>>> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
>>>> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
>>>> +
>>>> + struct B1 *outer1;
>>>> + struct C1 *outest1;
>>>> +
>>>> + /* Make sure optimization can't find some other object size. */
>>>> + outer1 = (void *)magic1;
>>>> + outest1 = (void *)magic2;
>>>> +
>>>> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
>>>> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
>>>> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
>>>> +
>>>> + struct Bn *outern;
>>>> + struct Cn *outestn;
>>>> +
>>>> + /* Make sure optimization can't find some other object size. */
>>>> + outern = (void *)magic1;
>>>> + outestn = (void *)magic2;
>>>> +
>>>> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
>>>> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
>>>> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
>>>> +
>>>> + DONE ();
>>>> + return 0;
>>>> +}
>>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>>>> index acd8deea34e..705d5702b9c 100644
>>>> --- a/gcc/tree-core.h
>>>> +++ b/gcc/tree-core.h
>>>> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
>>>> unsigned empty_flag : 1;
>>>> unsigned indivisible_p : 1;
>>>> RECORD_OR_UNION_CHECK
>>>
>>> it looks like the bit could be eventually shared with
>>> no_named_args_stdarg_p (but its accessor doesn't use
>>> FUNC_OR_METHOD_CHECK)
>> You mean to let “type_include_flexarray” share the same bit with “no_named_args_stdarg_p” in order to save 1-bit space in IR?
>> Then change the
>>
>> /* True if this is a stdarg function with no named arguments (C2x
>> (...) prototype, where arguments can be accessed with va_start and
>> va_arg), as opposed to an unprototyped function. */
>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>
>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>> at the last field recursively. */
>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>> (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>>
>> To:
>>
>> union {
>> unsigned no_named_args_stdarg_p : 1;
>> /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
>> unsigned int type_include_flexarray : 1;
>> } shared_bit;
>> unsigned spare : 15;
>>
>> /* True if this is a stdarg function with no named arguments (C2x
>> (...) prototype, where arguments can be accessed with va_start and
>> va_arg), as opposed to an unprototyped function. */
>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>> (FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
>>
>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>> at the last field recursively. */
>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>> (RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)
>
> No, we're just overloading bits by using the same name for different
> purposes. I don't think such a union would pack nicely. We overload
> by documenting the uses, in the above cases the uses are separated
> by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE
> and the accessor macros should be updated to use the appropriate
> tree check macros covering that so we don't try to inspect the
> "wrong bit”.
Okay, I see. Then should I change the name of that bit to one that reflect two usages?
>
>>>
>>>> alias_set_type alias_set;
>>>> tree pointer_to;
>>>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
>>>> index 9a936a91983..22b3c72ea6e 100644
>>>> --- a/gcc/tree-object-size.cc
>>>> +++ b/gcc/tree-object-size.cc
>>>> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
>>>> v = NULL_TREE;
>>>> break;
>>>> case COMPONENT_REF:
>>>> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>>>> + /* When the ref is not to an array, a record or a union, it
>>>> + will not have flexible size, compute the object size
>>>> + directly. */
>>>> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>>>> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
>>>> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
>>>> {
>>>> v = NULL_TREE;
>>>> break;
>>>> }
>>>> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> - != UNION_TYPE
>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> - != QUAL_UNION_TYPE)
>>>> - break;
>>>> - else
>>>> - v = TREE_OPERAND (v, 0);
>>>> - if (TREE_CODE (v) == COMPONENT_REF
>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> - == RECORD_TYPE)
>>>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
>>>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
>>>> + /* if the record or union does not include a flexible array
>>>> + recursively, compute the object size directly. */
>>>> {
>>>> - /* compute object size only if v is not a
>>>> - flexible array member. */
>>>> - if (!is_flexible_array_mem_ref)
>>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>>>> {
>>>> v = NULL_TREE;
>>>> break;
>>>> }
>>>> - v = TREE_OPERAND (v, 0);
>>>> + else
>>>> + v = TREE_OPERAND (v, 0);
>>>> }
>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> - != UNION_TYPE
>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> - != QUAL_UNION_TYPE)
>>>> - break;
>>>> - else
>>>> - v = TREE_OPERAND (v, 0);
>>>> - if (v != pt_var)
>>>> - v = NULL_TREE;
>>>> else
>>>> - v = pt_var;
>>>> + {
>>>> + /* Now the ref is to an array type. */
>>>> + is_flexible_array_mem_ref
>>>> + = array_ref_flexible_size_p (v);
>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> + != UNION_TYPE
>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> + != QUAL_UNION_TYPE)
>>>> + break;
>>>> + else
>>>> + v = TREE_OPERAND (v, 0);
>>>> + if (TREE_CODE (v) == COMPONENT_REF
>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> + == RECORD_TYPE)
>>>> + {
>>>> + /* compute object size only if v is not a
>>>> + flexible array member. */
>>>> + if (!is_flexible_array_mem_ref)
>>>> + {
>>>> + v = NULL_TREE;
>>>> + break;
>>>> + }
>>>> + v = TREE_OPERAND (v, 0);
>>>> + }
>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> + != UNION_TYPE
>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>> + != QUAL_UNION_TYPE)
>>>> + break;
>>>> + else
>>>> + v = TREE_OPERAND (v, 0);
>>>> + if (v != pt_var)
>>>> + v = NULL_TREE;
>>>> + else
>>>> + v = pt_var;
>>>> + }
>>>> break;
>>>> default:
>>>> v = pt_var;
>>>> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
>>>> index d4dc30f048f..c19ede0631d 100644
>>>> --- a/gcc/tree-streamer-in.cc
>>>> +++ b/gcc/tree-streamer-in.cc
>>>> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>>>> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>> }
>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>>>> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
>>>> index d107229da5c..73e4b4e547c 100644
>>>> --- a/gcc/tree-streamer-out.cc
>>>> +++ b/gcc/tree-streamer-out.cc
>>>> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>>>> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
>>>> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
>>>> : TYPE_CXX_ODR_P (expr), 1);
>>>> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
>>>> }
>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>>>> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
>>>> diff --git a/gcc/tree.h b/gcc/tree.h
>>>> index 92ac0e6a214..ab1cdc3dc85 100644
>>>> --- a/gcc/tree.h
>>>> +++ b/gcc/tree.h
>>>> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
>>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>>>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>>>
>>>> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>>>> + at the last field recursively. */
>>>> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
>>>> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>>>
>>> Please use RECORD_OR_UNION_CHECK.
>> Okay.
>>> The comment "at the last field"
>>> doesn't seem to match implementation? (at least not obviously)
>> The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the last field. (And recursively).
>>> Given this is a generic header expanding on what a "flexible array member"
>>> is to the middle-end here would be good. Especially the guarantees
>>> made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
>>> vs. DECL_FLEXARRAY).
>>
>> The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex array by default.
>> It’s only set to TRUE when the struct/union includes a flex array at the last field (and recursively). So, I think it should be correct.
>> Let me know if I missed anything here.
>
> See above - I was looking at the use (but I'm not familiar with that
> code), and wondered how the change affects uses from other frontends.
Please see my explanation above, I think the default behavior from other FE should be kept correctly with this patch.
thanks.
Qing
>
> Richard.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [V4][PATCH 2/2] Update documentation to clarify a GCC extension
2023-03-03 0:03 ` Qing Zhao
@ 2023-03-12 23:14 ` Sandra Loosemore
2023-03-13 12:46 ` Qing Zhao
0 siblings, 1 reply; 18+ messages in thread
From: Sandra Loosemore @ 2023-03-12 23:14 UTC (permalink / raw)
To: Qing Zhao, Joseph Myers, Richard Biener; +Cc: siddhesh, keescook, gcc-patches
On 3/2/23 17:03, Qing Zhao via Gcc-patches wrote:
> Ping.
It looks to me like there is an associated code patch (for PR101832)
that is still under technical discussion? Or is this documentation
patch independent of that change?
-Sandra
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [V4][PATCH 2/2] Update documentation to clarify a GCC extension
2023-03-12 23:14 ` Sandra Loosemore
@ 2023-03-13 12:46 ` Qing Zhao
0 siblings, 0 replies; 18+ messages in thread
From: Qing Zhao @ 2023-03-13 12:46 UTC (permalink / raw)
To: Sandra Loosemore
Cc: Joseph Myers, Richard Biener, siddhesh, keescook, gcc-patches
> On Mar 12, 2023, at 7:14 PM, Sandra Loosemore <sandra@codesourcery.com> wrote:
>
> On 3/2/23 17:03, Qing Zhao via Gcc-patches wrote:
>> Ping.
>
> It looks to me like there is an associated code patch (for PR101832) that is still under technical discussion?
Yes, the 1st patch in this serie is the patch for PR101832. Which is under review.
This 2nd patch is the associated doc change.
> Or is this documentation patch independent of that change?
These two patches are better to be associated together. But I think the patch for PR101832 might be
put in first into GCC13 if people think the doc change is risky at this moment.
Qing
>
> -Sandra
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-03-10 13:48 ` Qing Zhao
@ 2023-03-13 22:42 ` Qing Zhao
2023-03-14 9:04 ` Richard Biener
0 siblings, 1 reply; 18+ messages in thread
From: Qing Zhao @ 2023-03-13 22:42 UTC (permalink / raw)
To: Richard Biener; +Cc: joseph, siddhesh, keescook, gcc-patches
Hi, Richard,
Do you have more comments on my responds to your previous questions?
thanks.
Qing
> On Mar 10, 2023, at 8:48 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>
>
>
>> On Mar 10, 2023, at 2:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>
>> On Thu, 9 Mar 2023, Qing Zhao wrote:
>>
>>>
>>>
>>>> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>
>>>> On Fri, 24 Feb 2023, Qing Zhao wrote:
>>>>
>>>>> GCC extension accepts the case when a struct with a C99 flexible array member
>>>>> is embedded into another struct or union (possibly recursively).
>>>>> __builtin_object_size should treat such struct as flexible size.
>>>>>
>>>>> gcc/c/ChangeLog:
>>>>>
>>>>> PR tree-optimization/101832
>>>>> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
>>>>> struct/union type.
>>>>
>>>> I can't really comment on the correctness of this part but since
>>>> only the C frontend will ever set this and you are using it from
>>>> addr_object_size which is also used for other C family languages
>>>> (at least), I wonder if you can really test
>>>>
>>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>>>>
>>>> there.
>>>
>>> You mean for C++ and also other C family languages (other than C), the above bit cannot be set?
>>> Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY, which is only set for C too.
>>> So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set TYPE_INCLUDE_FLEXARRAY also in C++ FE?
>>> What’s your suggestion?
>>>
>>> (I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be set in the same places).
>>
>> I was wondering if the above test errors on the conservative side
>> correctly - it will now, for all but C, cut off some thing where it
>> didn't before?
>
> As long as the default value of TYPE_INCLUDE_FLEXARRAY reflects the correct conservative behavior, then the testing should be correct, right?
>
> The default value of TYPE_INCLUDE_FLEXARRAY is 0, i.e, FALSE, means that the TYPE does not include a flex array by default, this is the correct behavior. Only when the TYPE does include a flexiarray, it will be set to TRUE. So, I think it’s correct.
>
> This is a different situation for DECL_NOT_FLEXARRAY, by default, the compiler will treat ALL trailing arrays as FMA, so in order to keep the correct conservative behavior, we should keep the default value for DECL_NOT_FLEXARRAY as it’s a FMA, i.e, DECL_NOT_FLEXARRAY being FALSE, by default. Only when the array is NOT a FMA, we set it to true.
>
> So, the default value for TYPE_INCLUDE_FLEXARRAY is correct.
>>
>>>>
>>>> Originally I was suggesting to set this flag in stor-layout.cc
>>>> which eventually all languages funnel their types through and
>>>> if there's language specific handling use a langhook (with the
>>>> default implementation preserving the status quo).
>>>
>>> If we decide to set the bits in stor-layout.cc, where is the best place to do it? I checked the star-layout.cc code, looks like “layout_type” might be the place where we can set these bits for RECORD_TYPE, UNION_TYPE?
>>
>> Yes, it would be layout_type.
>>
>>>>
>>>> Some more comments below ...
>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>> PR tree-optimization/101832
>>>>> * module.cc (trees_out::core_bools): Stream out new bit
>>>>> type_include_flexarray.
>>>>> (trees_in::core_bools): Stream in new bit type_include_flexarray.
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> PR tree-optimization/101832
>>>>> * print-tree.cc (print_node): Print new bit type_include_flexarray.
>>>>> * tree-core.h (struct tree_type_common): New bit
>>>>> type_include_flexarray.
>>>>> * tree-object-size.cc (addr_object_size): Handle structure/union type
>>>>> when it has flexible size.
>>>>> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
>>>>> in new bit type_include_flexarray.
>>>>> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
>>>>> out new bit type_include_flexarray.
>>>>> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
>>>>> TYPE_INCLUDE_FLEXARRAY.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> PR tree-optimization/101832
>>>>> * gcc.dg/builtin-object-size-pr101832.c: New test.
>>>>> ---
>>>>> gcc/c/c-decl.cc | 12 ++
>>>>> gcc/cp/module.cc | 2 +
>>>>> gcc/print-tree.cc | 5 +
>>>>> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
>>>>> gcc/tree-core.h | 4 +-
>>>>> gcc/tree-object-size.cc | 79 +++++++----
>>>>> gcc/tree-streamer-in.cc | 1 +
>>>>> gcc/tree-streamer-out.cc | 1 +
>>>>> gcc/tree.h | 6 +
>>>>> 9 files changed, 215 insertions(+), 29 deletions(-)
>>>>> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>>>
>>>>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
>>>>> index 08078eadeb8..f589a2f5192 100644
>>>>> --- a/gcc/c/c-decl.cc
>>>>> +++ b/gcc/c/c-decl.cc
>>>>> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
>>>>> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
>>>>> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
>>>>>
>>>>> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>>>>> + * when x is an array. */
>>>>> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
>>>>> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
>>>>> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>>>>> + when x is the last field. */
>>>>> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
>>>>> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
>>>>> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
>>>>> + && is_last_field)
>>>>> + TYPE_INCLUDE_FLEXARRAY (t) = true;
>>>>> +
>>>>> if (DECL_NAME (x)
>>>>> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
>>>>> saw_named_field = true;
>>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>>>>> index ac2fe66b080..c750361b704 100644
>>>>> --- a/gcc/cp/module.cc
>>>>> +++ b/gcc/cp/module.cc
>>>>> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
>>>>> WB (t->type_common.lang_flag_5);
>>>>> WB (t->type_common.lang_flag_6);
>>>>> WB (t->type_common.typeless_storage);
>>>>> + WB (t->type_common.type_include_flexarray);
>>>>> }
>>>>>
>>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>>>>> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
>>>>> RB (t->type_common.lang_flag_5);
>>>>> RB (t->type_common.lang_flag_6);
>>>>> RB (t->type_common.typeless_storage);
>>>>> + RB (t->type_common.type_include_flexarray);
>>>>> }
>>>>>
>>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>>>>> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
>>>>> index 1f3afcbbc86..efacdb7686f 100644
>>>>> --- a/gcc/print-tree.cc
>>>>> +++ b/gcc/print-tree.cc
>>>>> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
>>>>> && TYPE_CXX_ODR_P (node))
>>>>> fputs (" cxx-odr-p", file);
>>>>>
>>>>> + if ((code == RECORD_TYPE
>>>>> + || code == UNION_TYPE)
>>>>> + && TYPE_INCLUDE_FLEXARRAY (node))
>>>>> + fputs (" include-flexarray", file);
>>>>> +
>>>>> /* The transparent-union flag is used for different things in
>>>>> different nodes. */
>>>>> if ((code == UNION_TYPE || code == RECORD_TYPE)
>>>>> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>>> new file mode 100644
>>>>> index 00000000000..60078e11634
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>>> @@ -0,0 +1,134 @@
>>>>> +/* PR 101832:
>>>>> + GCC extension accepts the case when a struct with a C99 flexible array
>>>>> + member is embedded into another struct (possibly recursively).
>>>>> + __builtin_object_size will treat such struct as flexible size.
>>>>> + However, when a structure with non-C99 flexible array member, i.e, trailing
>>>>> + [0], [1], or [4], is embedded into anther struct, the stucture will not
>>>>> + be treated as flexible size. */
>>>>> +/* { dg-do run } */
>>>>> +/* { dg-options "-O2" } */
>>>>> +
>>>>> +#include "builtin-object-size-common.h"
>>>>> +
>>>>> +#define expect(p, _v) do { \
>>>>> + size_t v = _v; \
>>>>> + if (p == v) \
>>>>> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
>>>>> + else {\
>>>>> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
>>>>> + FAIL (); \
>>>>> + } \
>>>>> +} while (0);
>>>>> +
>>>>> +
>>>>> +struct A {
>>>>> + int n;
>>>>> + char data[];
>>>>> +};
>>>>> +
>>>>> +struct B {
>>>>> + int m;
>>>>> + struct A a;
>>>>> +};
>>>>> +
>>>>> +struct C {
>>>>> + int q;
>>>>> + struct B b;
>>>>> +};
>>>>> +
>>>>> +struct A0 {
>>>>> + int n;
>>>>> + char data[0];
>>>>> +};
>>>>> +
>>>>> +struct B0 {
>>>>> + int m;
>>>>> + struct A0 a;
>>>>> +};
>>>>> +
>>>>> +struct C0 {
>>>>> + int q;
>>>>> + struct B0 b;
>>>>> +};
>>>>> +
>>>>> +struct A1 {
>>>>> + int n;
>>>>> + char data[1];
>>>>> +};
>>>>> +
>>>>> +struct B1 {
>>>>> + int m;
>>>>> + struct A1 a;
>>>>> +};
>>>>> +
>>>>> +struct C1 {
>>>>> + int q;
>>>>> + struct B1 b;
>>>>> +};
>>>>> +
>>>>> +struct An {
>>>>> + int n;
>>>>> + char data[8];
>>>>> +};
>>>>> +
>>>>> +struct Bn {
>>>>> + int m;
>>>>> + struct An a;
>>>>> +};
>>>>> +
>>>>> +struct Cn {
>>>>> + int q;
>>>>> + struct Bn b;
>>>>> +};
>>>>> +
>>>>> +volatile void *magic1, *magic2;
>>>>> +
>>>>> +int main (int argc, char *argv[])
>>>>> +{
>>>>> + struct B *outer;
>>>>> + struct C *outest;
>>>>> +
>>>>> + /* Make sure optimization can't find some other object size. */
>>>>> + outer = (void *)magic1;
>>>>> + outest = (void *)magic2;
>>>>> +
>>>>> + expect (__builtin_object_size (&outer->a, 1), -1);
>>>>> + expect (__builtin_object_size (&outest->b, 1), -1);
>>>>> + expect (__builtin_object_size (&outest->b.a, 1), -1);
>>>>> +
>>>>> + struct B0 *outer0;
>>>>> + struct C0 *outest0;
>>>>> +
>>>>> + /* Make sure optimization can't find some other object size. */
>>>>> + outer0 = (void *)magic1;
>>>>> + outest0 = (void *)magic2;
>>>>> +
>>>>> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
>>>>> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
>>>>> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
>>>>> +
>>>>> + struct B1 *outer1;
>>>>> + struct C1 *outest1;
>>>>> +
>>>>> + /* Make sure optimization can't find some other object size. */
>>>>> + outer1 = (void *)magic1;
>>>>> + outest1 = (void *)magic2;
>>>>> +
>>>>> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
>>>>> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
>>>>> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
>>>>> +
>>>>> + struct Bn *outern;
>>>>> + struct Cn *outestn;
>>>>> +
>>>>> + /* Make sure optimization can't find some other object size. */
>>>>> + outern = (void *)magic1;
>>>>> + outestn = (void *)magic2;
>>>>> +
>>>>> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
>>>>> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
>>>>> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
>>>>> +
>>>>> + DONE ();
>>>>> + return 0;
>>>>> +}
>>>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>>>>> index acd8deea34e..705d5702b9c 100644
>>>>> --- a/gcc/tree-core.h
>>>>> +++ b/gcc/tree-core.h
>>>>> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
>>>>> unsigned empty_flag : 1;
>>>>> unsigned indivisible_p : 1;
>>>>> RECORD_OR_UNION_CHECK
>>>>
>>>> it looks like the bit could be eventually shared with
>>>> no_named_args_stdarg_p (but its accessor doesn't use
>>>> FUNC_OR_METHOD_CHECK)
>>> You mean to let “type_include_flexarray” share the same bit with “no_named_args_stdarg_p” in order to save 1-bit space in IR?
>>> Then change the
>>>
>>> /* True if this is a stdarg function with no named arguments (C2x
>>> (...) prototype, where arguments can be accessed with va_start and
>>> va_arg), as opposed to an unprototyped function. */
>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>>
>>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>>> at the last field recursively. */
>>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>>> (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>>>
>>> To:
>>>
>>> union {
>>> unsigned no_named_args_stdarg_p : 1;
>>> /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
>>> unsigned int type_include_flexarray : 1;
>>> } shared_bit;
>>> unsigned spare : 15;
>>>
>>> /* True if this is a stdarg function with no named arguments (C2x
>>> (...) prototype, where arguments can be accessed with va_start and
>>> va_arg), as opposed to an unprototyped function. */
>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>>> (FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
>>>
>>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>>> at the last field recursively. */
>>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>>> (RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)
>>
>> No, we're just overloading bits by using the same name for different
>> purposes. I don't think such a union would pack nicely. We overload
>> by documenting the uses, in the above cases the uses are separated
>> by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE
>> and the accessor macros should be updated to use the appropriate
>> tree check macros covering that so we don't try to inspect the
>> "wrong bit”.
> Okay, I see. Then should I change the name of that bit to one that reflect two usages?
>>
>>>>
>>>>> alias_set_type alias_set;
>>>>> tree pointer_to;
>>>>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
>>>>> index 9a936a91983..22b3c72ea6e 100644
>>>>> --- a/gcc/tree-object-size.cc
>>>>> +++ b/gcc/tree-object-size.cc
>>>>> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
>>>>> v = NULL_TREE;
>>>>> break;
>>>>> case COMPONENT_REF:
>>>>> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>>>>> + /* When the ref is not to an array, a record or a union, it
>>>>> + will not have flexible size, compute the object size
>>>>> + directly. */
>>>>> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>>>>> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
>>>>> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
>>>>> {
>>>>> v = NULL_TREE;
>>>>> break;
>>>>> }
>>>>> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
>>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> - != UNION_TYPE
>>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> - != QUAL_UNION_TYPE)
>>>>> - break;
>>>>> - else
>>>>> - v = TREE_OPERAND (v, 0);
>>>>> - if (TREE_CODE (v) == COMPONENT_REF
>>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> - == RECORD_TYPE)
>>>>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
>>>>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
>>>>> + /* if the record or union does not include a flexible array
>>>>> + recursively, compute the object size directly. */
>>>>> {
>>>>> - /* compute object size only if v is not a
>>>>> - flexible array member. */
>>>>> - if (!is_flexible_array_mem_ref)
>>>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>>>>> {
>>>>> v = NULL_TREE;
>>>>> break;
>>>>> }
>>>>> - v = TREE_OPERAND (v, 0);
>>>>> + else
>>>>> + v = TREE_OPERAND (v, 0);
>>>>> }
>>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> - != UNION_TYPE
>>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> - != QUAL_UNION_TYPE)
>>>>> - break;
>>>>> - else
>>>>> - v = TREE_OPERAND (v, 0);
>>>>> - if (v != pt_var)
>>>>> - v = NULL_TREE;
>>>>> else
>>>>> - v = pt_var;
>>>>> + {
>>>>> + /* Now the ref is to an array type. */
>>>>> + is_flexible_array_mem_ref
>>>>> + = array_ref_flexible_size_p (v);
>>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> + != UNION_TYPE
>>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> + != QUAL_UNION_TYPE)
>>>>> + break;
>>>>> + else
>>>>> + v = TREE_OPERAND (v, 0);
>>>>> + if (TREE_CODE (v) == COMPONENT_REF
>>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> + == RECORD_TYPE)
>>>>> + {
>>>>> + /* compute object size only if v is not a
>>>>> + flexible array member. */
>>>>> + if (!is_flexible_array_mem_ref)
>>>>> + {
>>>>> + v = NULL_TREE;
>>>>> + break;
>>>>> + }
>>>>> + v = TREE_OPERAND (v, 0);
>>>>> + }
>>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> + != UNION_TYPE
>>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>> + != QUAL_UNION_TYPE)
>>>>> + break;
>>>>> + else
>>>>> + v = TREE_OPERAND (v, 0);
>>>>> + if (v != pt_var)
>>>>> + v = NULL_TREE;
>>>>> + else
>>>>> + v = pt_var;
>>>>> + }
>>>>> break;
>>>>> default:
>>>>> v = pt_var;
>>>>> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
>>>>> index d4dc30f048f..c19ede0631d 100644
>>>>> --- a/gcc/tree-streamer-in.cc
>>>>> +++ b/gcc/tree-streamer-in.cc
>>>>> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>>>>> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>> }
>>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>>>>> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
>>>>> index d107229da5c..73e4b4e547c 100644
>>>>> --- a/gcc/tree-streamer-out.cc
>>>>> +++ b/gcc/tree-streamer-out.cc
>>>>> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>>>>> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
>>>>> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
>>>>> : TYPE_CXX_ODR_P (expr), 1);
>>>>> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
>>>>> }
>>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>>>>> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
>>>>> diff --git a/gcc/tree.h b/gcc/tree.h
>>>>> index 92ac0e6a214..ab1cdc3dc85 100644
>>>>> --- a/gcc/tree.h
>>>>> +++ b/gcc/tree.h
>>>>> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
>>>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>>>>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>>>>
>>>>> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>>>>> + at the last field recursively. */
>>>>> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
>>>>> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>>>>
>>>> Please use RECORD_OR_UNION_CHECK.
>>> Okay.
>>>> The comment "at the last field"
>>>> doesn't seem to match implementation? (at least not obviously)
>>> The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the last field. (And recursively).
>>>> Given this is a generic header expanding on what a "flexible array member"
>>>> is to the middle-end here would be good. Especially the guarantees
>>>> made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
>>>> vs. DECL_FLEXARRAY).
>>>
>>> The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex array by default.
>>> It’s only set to TRUE when the struct/union includes a flex array at the last field (and recursively). So, I think it should be correct.
>>> Let me know if I missed anything here.
>>
>> See above - I was looking at the use (but I'm not familiar with that
>> code), and wondered how the change affects uses from other frontends.
>
> Please see my explanation above, I think the default behavior from other FE should be kept correctly with this patch.
>
> thanks.
>
> Qing
>>
>> Richard.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-03-13 22:42 ` Qing Zhao
@ 2023-03-14 9:04 ` Richard Biener
2023-03-14 13:09 ` Qing Zhao
0 siblings, 1 reply; 18+ messages in thread
From: Richard Biener @ 2023-03-14 9:04 UTC (permalink / raw)
To: Qing Zhao; +Cc: joseph, siddhesh, keescook, gcc-patches
[-- Attachment #1: Type: text/plain, Size: 22752 bytes --]
On Mon, 13 Mar 2023, Qing Zhao wrote:
> Hi, Richard,
>
> Do you have more comments on my responds to your previous questions?
No, generally we're not good at naming the shared bits, so keeping the
old name is fine here.
Btw, I do not feel competent enough to approve the patch, instead that's
on the burden of the C frontend maintainers and the people understanding
the object-size code more.
Richard.
> thanks.
>
> Qing
>
> > On Mar 10, 2023, at 8:48 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> >
> >
> >
> >> On Mar 10, 2023, at 2:54 AM, Richard Biener <rguenther@suse.de> wrote:
> >>
> >> On Thu, 9 Mar 2023, Qing Zhao wrote:
> >>
> >>>
> >>>
> >>>> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@suse.de> wrote:
> >>>>
> >>>> On Fri, 24 Feb 2023, Qing Zhao wrote:
> >>>>
> >>>>> GCC extension accepts the case when a struct with a C99 flexible array member
> >>>>> is embedded into another struct or union (possibly recursively).
> >>>>> __builtin_object_size should treat such struct as flexible size.
> >>>>>
> >>>>> gcc/c/ChangeLog:
> >>>>>
> >>>>> PR tree-optimization/101832
> >>>>> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
> >>>>> struct/union type.
> >>>>
> >>>> I can't really comment on the correctness of this part but since
> >>>> only the C frontend will ever set this and you are using it from
> >>>> addr_object_size which is also used for other C family languages
> >>>> (at least), I wonder if you can really test
> >>>>
> >>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> >>>>
> >>>> there.
> >>>
> >>> You mean for C++ and also other C family languages (other than C), the above bit cannot be set?
> >>> Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY, which is only set for C too.
> >>> So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set TYPE_INCLUDE_FLEXARRAY also in C++ FE?
> >>> What’s your suggestion?
> >>>
> >>> (I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be set in the same places).
> >>
> >> I was wondering if the above test errors on the conservative side
> >> correctly - it will now, for all but C, cut off some thing where it
> >> didn't before?
> >
> > As long as the default value of TYPE_INCLUDE_FLEXARRAY reflects the correct conservative behavior, then the testing should be correct, right?
> >
> > The default value of TYPE_INCLUDE_FLEXARRAY is 0, i.e, FALSE, means that the TYPE does not include a flex array by default, this is the correct behavior. Only when the TYPE does include a flexiarray, it will be set to TRUE. So, I think it’s correct.
> >
> > This is a different situation for DECL_NOT_FLEXARRAY, by default, the compiler will treat ALL trailing arrays as FMA, so in order to keep the correct conservative behavior, we should keep the default value for DECL_NOT_FLEXARRAY as it’s a FMA, i.e, DECL_NOT_FLEXARRAY being FALSE, by default. Only when the array is NOT a FMA, we set it to true.
> >
> > So, the default value for TYPE_INCLUDE_FLEXARRAY is correct.
> >>
> >>>>
> >>>> Originally I was suggesting to set this flag in stor-layout.cc
> >>>> which eventually all languages funnel their types through and
> >>>> if there's language specific handling use a langhook (with the
> >>>> default implementation preserving the status quo).
> >>>
> >>> If we decide to set the bits in stor-layout.cc, where is the best place to do it? I checked the star-layout.cc code, looks like “layout_type” might be the place where we can set these bits for RECORD_TYPE, UNION_TYPE?
> >>
> >> Yes, it would be layout_type.
> >>
> >>>>
> >>>> Some more comments below ...
> >>>>
> >>>>> gcc/cp/ChangeLog:
> >>>>>
> >>>>> PR tree-optimization/101832
> >>>>> * module.cc (trees_out::core_bools): Stream out new bit
> >>>>> type_include_flexarray.
> >>>>> (trees_in::core_bools): Stream in new bit type_include_flexarray.
> >>>>>
> >>>>> gcc/ChangeLog:
> >>>>>
> >>>>> PR tree-optimization/101832
> >>>>> * print-tree.cc (print_node): Print new bit type_include_flexarray.
> >>>>> * tree-core.h (struct tree_type_common): New bit
> >>>>> type_include_flexarray.
> >>>>> * tree-object-size.cc (addr_object_size): Handle structure/union type
> >>>>> when it has flexible size.
> >>>>> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
> >>>>> in new bit type_include_flexarray.
> >>>>> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
> >>>>> out new bit type_include_flexarray.
> >>>>> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
> >>>>> TYPE_INCLUDE_FLEXARRAY.
> >>>>>
> >>>>> gcc/testsuite/ChangeLog:
> >>>>>
> >>>>> PR tree-optimization/101832
> >>>>> * gcc.dg/builtin-object-size-pr101832.c: New test.
> >>>>> ---
> >>>>> gcc/c/c-decl.cc | 12 ++
> >>>>> gcc/cp/module.cc | 2 +
> >>>>> gcc/print-tree.cc | 5 +
> >>>>> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
> >>>>> gcc/tree-core.h | 4 +-
> >>>>> gcc/tree-object-size.cc | 79 +++++++----
> >>>>> gcc/tree-streamer-in.cc | 1 +
> >>>>> gcc/tree-streamer-out.cc | 1 +
> >>>>> gcc/tree.h | 6 +
> >>>>> 9 files changed, 215 insertions(+), 29 deletions(-)
> >>>>> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >>>>>
> >>>>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
> >>>>> index 08078eadeb8..f589a2f5192 100644
> >>>>> --- a/gcc/c/c-decl.cc
> >>>>> +++ b/gcc/c/c-decl.cc
> >>>>> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
> >>>>> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
> >>>>> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
> >>>>>
> >>>>> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> >>>>> + * when x is an array. */
> >>>>> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
> >>>>> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
> >>>>> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
> >>>>> + when x is the last field. */
> >>>>> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
> >>>>> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
> >>>>> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
> >>>>> + && is_last_field)
> >>>>> + TYPE_INCLUDE_FLEXARRAY (t) = true;
> >>>>> +
> >>>>> if (DECL_NAME (x)
> >>>>> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> >>>>> saw_named_field = true;
> >>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> >>>>> index ac2fe66b080..c750361b704 100644
> >>>>> --- a/gcc/cp/module.cc
> >>>>> +++ b/gcc/cp/module.cc
> >>>>> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
> >>>>> WB (t->type_common.lang_flag_5);
> >>>>> WB (t->type_common.lang_flag_6);
> >>>>> WB (t->type_common.typeless_storage);
> >>>>> + WB (t->type_common.type_include_flexarray);
> >>>>> }
> >>>>>
> >>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> >>>>> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
> >>>>> RB (t->type_common.lang_flag_5);
> >>>>> RB (t->type_common.lang_flag_6);
> >>>>> RB (t->type_common.typeless_storage);
> >>>>> + RB (t->type_common.type_include_flexarray);
> >>>>> }
> >>>>>
> >>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
> >>>>> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
> >>>>> index 1f3afcbbc86..efacdb7686f 100644
> >>>>> --- a/gcc/print-tree.cc
> >>>>> +++ b/gcc/print-tree.cc
> >>>>> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
> >>>>> && TYPE_CXX_ODR_P (node))
> >>>>> fputs (" cxx-odr-p", file);
> >>>>>
> >>>>> + if ((code == RECORD_TYPE
> >>>>> + || code == UNION_TYPE)
> >>>>> + && TYPE_INCLUDE_FLEXARRAY (node))
> >>>>> + fputs (" include-flexarray", file);
> >>>>> +
> >>>>> /* The transparent-union flag is used for different things in
> >>>>> different nodes. */
> >>>>> if ((code == UNION_TYPE || code == RECORD_TYPE)
> >>>>> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >>>>> new file mode 100644
> >>>>> index 00000000000..60078e11634
> >>>>> --- /dev/null
> >>>>> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> >>>>> @@ -0,0 +1,134 @@
> >>>>> +/* PR 101832:
> >>>>> + GCC extension accepts the case when a struct with a C99 flexible array
> >>>>> + member is embedded into another struct (possibly recursively).
> >>>>> + __builtin_object_size will treat such struct as flexible size.
> >>>>> + However, when a structure with non-C99 flexible array member, i.e, trailing
> >>>>> + [0], [1], or [4], is embedded into anther struct, the stucture will not
> >>>>> + be treated as flexible size. */
> >>>>> +/* { dg-do run } */
> >>>>> +/* { dg-options "-O2" } */
> >>>>> +
> >>>>> +#include "builtin-object-size-common.h"
> >>>>> +
> >>>>> +#define expect(p, _v) do { \
> >>>>> + size_t v = _v; \
> >>>>> + if (p == v) \
> >>>>> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
> >>>>> + else {\
> >>>>> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
> >>>>> + FAIL (); \
> >>>>> + } \
> >>>>> +} while (0);
> >>>>> +
> >>>>> +
> >>>>> +struct A {
> >>>>> + int n;
> >>>>> + char data[];
> >>>>> +};
> >>>>> +
> >>>>> +struct B {
> >>>>> + int m;
> >>>>> + struct A a;
> >>>>> +};
> >>>>> +
> >>>>> +struct C {
> >>>>> + int q;
> >>>>> + struct B b;
> >>>>> +};
> >>>>> +
> >>>>> +struct A0 {
> >>>>> + int n;
> >>>>> + char data[0];
> >>>>> +};
> >>>>> +
> >>>>> +struct B0 {
> >>>>> + int m;
> >>>>> + struct A0 a;
> >>>>> +};
> >>>>> +
> >>>>> +struct C0 {
> >>>>> + int q;
> >>>>> + struct B0 b;
> >>>>> +};
> >>>>> +
> >>>>> +struct A1 {
> >>>>> + int n;
> >>>>> + char data[1];
> >>>>> +};
> >>>>> +
> >>>>> +struct B1 {
> >>>>> + int m;
> >>>>> + struct A1 a;
> >>>>> +};
> >>>>> +
> >>>>> +struct C1 {
> >>>>> + int q;
> >>>>> + struct B1 b;
> >>>>> +};
> >>>>> +
> >>>>> +struct An {
> >>>>> + int n;
> >>>>> + char data[8];
> >>>>> +};
> >>>>> +
> >>>>> +struct Bn {
> >>>>> + int m;
> >>>>> + struct An a;
> >>>>> +};
> >>>>> +
> >>>>> +struct Cn {
> >>>>> + int q;
> >>>>> + struct Bn b;
> >>>>> +};
> >>>>> +
> >>>>> +volatile void *magic1, *magic2;
> >>>>> +
> >>>>> +int main (int argc, char *argv[])
> >>>>> +{
> >>>>> + struct B *outer;
> >>>>> + struct C *outest;
> >>>>> +
> >>>>> + /* Make sure optimization can't find some other object size. */
> >>>>> + outer = (void *)magic1;
> >>>>> + outest = (void *)magic2;
> >>>>> +
> >>>>> + expect (__builtin_object_size (&outer->a, 1), -1);
> >>>>> + expect (__builtin_object_size (&outest->b, 1), -1);
> >>>>> + expect (__builtin_object_size (&outest->b.a, 1), -1);
> >>>>> +
> >>>>> + struct B0 *outer0;
> >>>>> + struct C0 *outest0;
> >>>>> +
> >>>>> + /* Make sure optimization can't find some other object size. */
> >>>>> + outer0 = (void *)magic1;
> >>>>> + outest0 = (void *)magic2;
> >>>>> +
> >>>>> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
> >>>>> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
> >>>>> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
> >>>>> +
> >>>>> + struct B1 *outer1;
> >>>>> + struct C1 *outest1;
> >>>>> +
> >>>>> + /* Make sure optimization can't find some other object size. */
> >>>>> + outer1 = (void *)magic1;
> >>>>> + outest1 = (void *)magic2;
> >>>>> +
> >>>>> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
> >>>>> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
> >>>>> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
> >>>>> +
> >>>>> + struct Bn *outern;
> >>>>> + struct Cn *outestn;
> >>>>> +
> >>>>> + /* Make sure optimization can't find some other object size. */
> >>>>> + outern = (void *)magic1;
> >>>>> + outestn = (void *)magic2;
> >>>>> +
> >>>>> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
> >>>>> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
> >>>>> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
> >>>>> +
> >>>>> + DONE ();
> >>>>> + return 0;
> >>>>> +}
> >>>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> >>>>> index acd8deea34e..705d5702b9c 100644
> >>>>> --- a/gcc/tree-core.h
> >>>>> +++ b/gcc/tree-core.h
> >>>>> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
> >>>>> unsigned empty_flag : 1;
> >>>>> unsigned indivisible_p : 1;
> >>>>> RECORD_OR_UNION_CHECK
> >>>>
> >>>> it looks like the bit could be eventually shared with
> >>>> no_named_args_stdarg_p (but its accessor doesn't use
> >>>> FUNC_OR_METHOD_CHECK)
> >>> You mean to let “type_include_flexarray” share the same bit with “no_named_args_stdarg_p” in order to save 1-bit space in IR?
> >>> Then change the
> >>>
> >>> /* True if this is a stdarg function with no named arguments (C2x
> >>> (...) prototype, where arguments can be accessed with va_start and
> >>> va_arg), as opposed to an unprototyped function. */
> >>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> >>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> >>>
> >>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> >>> at the last field recursively. */
> >>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
> >>> (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
> >>>
> >>> To:
> >>>
> >>> union {
> >>> unsigned no_named_args_stdarg_p : 1;
> >>> /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
> >>> unsigned int type_include_flexarray : 1;
> >>> } shared_bit;
> >>> unsigned spare : 15;
> >>>
> >>> /* True if this is a stdarg function with no named arguments (C2x
> >>> (...) prototype, where arguments can be accessed with va_start and
> >>> va_arg), as opposed to an unprototyped function. */
> >>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> >>> (FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
> >>>
> >>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> >>> at the last field recursively. */
> >>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
> >>> (RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)
> >>
> >> No, we're just overloading bits by using the same name for different
> >> purposes. I don't think such a union would pack nicely. We overload
> >> by documenting the uses, in the above cases the uses are separated
> >> by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE
> >> and the accessor macros should be updated to use the appropriate
> >> tree check macros covering that so we don't try to inspect the
> >> "wrong bit”.
> > Okay, I see. Then should I change the name of that bit to one that reflect two usages?
> >>
> >>>>
> >>>>> alias_set_type alias_set;
> >>>>> tree pointer_to;
> >>>>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
> >>>>> index 9a936a91983..22b3c72ea6e 100644
> >>>>> --- a/gcc/tree-object-size.cc
> >>>>> +++ b/gcc/tree-object-size.cc
> >>>>> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
> >>>>> v = NULL_TREE;
> >>>>> break;
> >>>>> case COMPONENT_REF:
> >>>>> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> >>>>> + /* When the ref is not to an array, a record or a union, it
> >>>>> + will not have flexible size, compute the object size
> >>>>> + directly. */
> >>>>> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> >>>>> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
> >>>>> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
> >>>>> {
> >>>>> v = NULL_TREE;
> >>>>> break;
> >>>>> }
> >>>>> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
> >>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> - != UNION_TYPE
> >>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> - != QUAL_UNION_TYPE)
> >>>>> - break;
> >>>>> - else
> >>>>> - v = TREE_OPERAND (v, 0);
> >>>>> - if (TREE_CODE (v) == COMPONENT_REF
> >>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> - == RECORD_TYPE)
> >>>>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
> >>>>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
> >>>>> + /* if the record or union does not include a flexible array
> >>>>> + recursively, compute the object size directly. */
> >>>>> {
> >>>>> - /* compute object size only if v is not a
> >>>>> - flexible array member. */
> >>>>> - if (!is_flexible_array_mem_ref)
> >>>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
> >>>>> {
> >>>>> v = NULL_TREE;
> >>>>> break;
> >>>>> }
> >>>>> - v = TREE_OPERAND (v, 0);
> >>>>> + else
> >>>>> + v = TREE_OPERAND (v, 0);
> >>>>> }
> >>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> - != UNION_TYPE
> >>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> - != QUAL_UNION_TYPE)
> >>>>> - break;
> >>>>> - else
> >>>>> - v = TREE_OPERAND (v, 0);
> >>>>> - if (v != pt_var)
> >>>>> - v = NULL_TREE;
> >>>>> else
> >>>>> - v = pt_var;
> >>>>> + {
> >>>>> + /* Now the ref is to an array type. */
> >>>>> + is_flexible_array_mem_ref
> >>>>> + = array_ref_flexible_size_p (v);
> >>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> + != UNION_TYPE
> >>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> + != QUAL_UNION_TYPE)
> >>>>> + break;
> >>>>> + else
> >>>>> + v = TREE_OPERAND (v, 0);
> >>>>> + if (TREE_CODE (v) == COMPONENT_REF
> >>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> + == RECORD_TYPE)
> >>>>> + {
> >>>>> + /* compute object size only if v is not a
> >>>>> + flexible array member. */
> >>>>> + if (!is_flexible_array_mem_ref)
> >>>>> + {
> >>>>> + v = NULL_TREE;
> >>>>> + break;
> >>>>> + }
> >>>>> + v = TREE_OPERAND (v, 0);
> >>>>> + }
> >>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
> >>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> + != UNION_TYPE
> >>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> >>>>> + != QUAL_UNION_TYPE)
> >>>>> + break;
> >>>>> + else
> >>>>> + v = TREE_OPERAND (v, 0);
> >>>>> + if (v != pt_var)
> >>>>> + v = NULL_TREE;
> >>>>> + else
> >>>>> + v = pt_var;
> >>>>> + }
> >>>>> break;
> >>>>> default:
> >>>>> v = pt_var;
> >>>>> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
> >>>>> index d4dc30f048f..c19ede0631d 100644
> >>>>> --- a/gcc/tree-streamer-in.cc
> >>>>> +++ b/gcc/tree-streamer-in.cc
> >>>>> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> >>>>> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>>>> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>>>> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>>>> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>>>> }
> >>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
> >>>>> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
> >>>>> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
> >>>>> index d107229da5c..73e4b4e547c 100644
> >>>>> --- a/gcc/tree-streamer-out.cc
> >>>>> +++ b/gcc/tree-streamer-out.cc
> >>>>> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
> >>>>> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
> >>>>> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
> >>>>> : TYPE_CXX_ODR_P (expr), 1);
> >>>>> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
> >>>>> }
> >>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
> >>>>> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
> >>>>> diff --git a/gcc/tree.h b/gcc/tree.h
> >>>>> index 92ac0e6a214..ab1cdc3dc85 100644
> >>>>> --- a/gcc/tree.h
> >>>>> +++ b/gcc/tree.h
> >>>>> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
> >>>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> >>>>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> >>>>>
> >>>>> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> >>>>> + at the last field recursively. */
> >>>>> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
> >>>>> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
> >>>>
> >>>> Please use RECORD_OR_UNION_CHECK.
> >>> Okay.
> >>>> The comment "at the last field"
> >>>> doesn't seem to match implementation? (at least not obviously)
> >>> The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the last field. (And recursively).
> >>>> Given this is a generic header expanding on what a "flexible array member"
> >>>> is to the middle-end here would be good. Especially the guarantees
> >>>> made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
> >>>> vs. DECL_FLEXARRAY).
> >>>
> >>> The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex array by default.
> >>> It’s only set to TRUE when the struct/union includes a flex array at the last field (and recursively). So, I think it should be correct.
> >>> Let me know if I missed anything here.
> >>
> >> See above - I was looking at the use (but I'm not familiar with that
> >> code), and wondered how the change affects uses from other frontends.
> >
> > Please see my explanation above, I think the default behavior from other FE should be kept correctly with this patch.
> >
> > thanks.
> >
> > Qing
> >>
> >> Richard.
>
>
--
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)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832]
2023-03-14 9:04 ` Richard Biener
@ 2023-03-14 13:09 ` Qing Zhao
0 siblings, 0 replies; 18+ messages in thread
From: Qing Zhao @ 2023-03-14 13:09 UTC (permalink / raw)
To: Richard Biener, Joseph Myers, Jakub Jelinek
Cc: siddhesh, keescook, gcc-patches
> On Mar 14, 2023, at 5:04 AM, Richard Biener <rguenther@suse.de> wrote:
>
> On Mon, 13 Mar 2023, Qing Zhao wrote:
>
>> Hi, Richard,
>>
>> Do you have more comments on my responds to your previous questions?
>
> No, generally we're not good at naming the shared bits, so keeping the
> old name is fine here.
Okay. I will keep the old name for it.
>
> Btw, I do not feel competent enough to approve the patch, instead that's
> on the burden of the C frontend maintainers and the people understanding
> the object-size code more.
So, Joseph and Jakub should be the ones to approve these patches?
I will update the patches with your suggestions for the bit.
And send the 5th version .
Thanks.
Qing
>
> Richard.
>
>> thanks.
>>
>> Qing
>>
>>> On Mar 10, 2023, at 8:48 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>>>
>>>
>>>
>>>> On Mar 10, 2023, at 2:54 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>
>>>> On Thu, 9 Mar 2023, Qing Zhao wrote:
>>>>
>>>>>
>>>>>
>>>>>> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguenther@suse.de> wrote:
>>>>>>
>>>>>> On Fri, 24 Feb 2023, Qing Zhao wrote:
>>>>>>
>>>>>>> GCC extension accepts the case when a struct with a C99 flexible array member
>>>>>>> is embedded into another struct or union (possibly recursively).
>>>>>>> __builtin_object_size should treat such struct as flexible size.
>>>>>>>
>>>>>>> gcc/c/ChangeLog:
>>>>>>>
>>>>>>> PR tree-optimization/101832
>>>>>>> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for
>>>>>>> struct/union type.
>>>>>>
>>>>>> I can't really comment on the correctness of this part but since
>>>>>> only the C frontend will ever set this and you are using it from
>>>>>> addr_object_size which is also used for other C family languages
>>>>>> (at least), I wonder if you can really test
>>>>>>
>>>>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>>>>>>
>>>>>> there.
>>>>>
>>>>> You mean for C++ and also other C family languages (other than C), the above bit cannot be set?
>>>>> Yes, that’s true. The bit is only set for C. So is the bit DECL_NOT_FLEXARRAY, which is only set for C too.
>>>>> So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can set TYPE_INCLUDE_FLEXARRAY also in C++ FE?
>>>>> What’s your suggestion?
>>>>>
>>>>> (I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should be set in the same places).
>>>>
>>>> I was wondering if the above test errors on the conservative side
>>>> correctly - it will now, for all but C, cut off some thing where it
>>>> didn't before?
>>>
>>> As long as the default value of TYPE_INCLUDE_FLEXARRAY reflects the correct conservative behavior, then the testing should be correct, right?
>>>
>>> The default value of TYPE_INCLUDE_FLEXARRAY is 0, i.e, FALSE, means that the TYPE does not include a flex array by default, this is the correct behavior. Only when the TYPE does include a flexiarray, it will be set to TRUE. So, I think it’s correct.
>>>
>>> This is a different situation for DECL_NOT_FLEXARRAY, by default, the compiler will treat ALL trailing arrays as FMA, so in order to keep the correct conservative behavior, we should keep the default value for DECL_NOT_FLEXARRAY as it’s a FMA, i.e, DECL_NOT_FLEXARRAY being FALSE, by default. Only when the array is NOT a FMA, we set it to true.
>>>
>>> So, the default value for TYPE_INCLUDE_FLEXARRAY is correct.
>>>>
>>>>>>
>>>>>> Originally I was suggesting to set this flag in stor-layout.cc
>>>>>> which eventually all languages funnel their types through and
>>>>>> if there's language specific handling use a langhook (with the
>>>>>> default implementation preserving the status quo).
>>>>>
>>>>> If we decide to set the bits in stor-layout.cc, where is the best place to do it? I checked the star-layout.cc code, looks like “layout_type” might be the place where we can set these bits for RECORD_TYPE, UNION_TYPE?
>>>>
>>>> Yes, it would be layout_type.
>>>>
>>>>>>
>>>>>> Some more comments below ...
>>>>>>
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>> PR tree-optimization/101832
>>>>>>> * module.cc (trees_out::core_bools): Stream out new bit
>>>>>>> type_include_flexarray.
>>>>>>> (trees_in::core_bools): Stream in new bit type_include_flexarray.
>>>>>>>
>>>>>>> gcc/ChangeLog:
>>>>>>>
>>>>>>> PR tree-optimization/101832
>>>>>>> * print-tree.cc (print_node): Print new bit type_include_flexarray.
>>>>>>> * tree-core.h (struct tree_type_common): New bit
>>>>>>> type_include_flexarray.
>>>>>>> * tree-object-size.cc (addr_object_size): Handle structure/union type
>>>>>>> when it has flexible size.
>>>>>>> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream
>>>>>>> in new bit type_include_flexarray.
>>>>>>> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream
>>>>>>> out new bit type_include_flexarray.
>>>>>>> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro
>>>>>>> TYPE_INCLUDE_FLEXARRAY.
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>
>>>>>>> PR tree-optimization/101832
>>>>>>> * gcc.dg/builtin-object-size-pr101832.c: New test.
>>>>>>> ---
>>>>>>> gcc/c/c-decl.cc | 12 ++
>>>>>>> gcc/cp/module.cc | 2 +
>>>>>>> gcc/print-tree.cc | 5 +
>>>>>>> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++
>>>>>>> gcc/tree-core.h | 4 +-
>>>>>>> gcc/tree-object-size.cc | 79 +++++++----
>>>>>>> gcc/tree-streamer-in.cc | 1 +
>>>>>>> gcc/tree-streamer-out.cc | 1 +
>>>>>>> gcc/tree.h | 6 +
>>>>>>> 9 files changed, 215 insertions(+), 29 deletions(-)
>>>>>>> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>>>>>
>>>>>>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
>>>>>>> index 08078eadeb8..f589a2f5192 100644
>>>>>>> --- a/gcc/c/c-decl.cc
>>>>>>> +++ b/gcc/c/c-decl.cc
>>>>>>> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
>>>>>>> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */
>>>>>>> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
>>>>>>>
>>>>>>> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>>>>>>> + * when x is an array. */
>>>>>>> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
>>>>>>> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE (x)) ;
>>>>>>> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t
>>>>>>> + when x is the last field. */
>>>>>>> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE
>>>>>>> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE)
>>>>>>> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x))
>>>>>>> + && is_last_field)
>>>>>>> + TYPE_INCLUDE_FLEXARRAY (t) = true;
>>>>>>> +
>>>>>>> if (DECL_NAME (x)
>>>>>>> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
>>>>>>> saw_named_field = true;
>>>>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>>>>>>> index ac2fe66b080..c750361b704 100644
>>>>>>> --- a/gcc/cp/module.cc
>>>>>>> +++ b/gcc/cp/module.cc
>>>>>>> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t)
>>>>>>> WB (t->type_common.lang_flag_5);
>>>>>>> WB (t->type_common.lang_flag_6);
>>>>>>> WB (t->type_common.typeless_storage);
>>>>>>> + WB (t->type_common.type_include_flexarray);
>>>>>>> }
>>>>>>>
>>>>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>>>>>>> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t)
>>>>>>> RB (t->type_common.lang_flag_5);
>>>>>>> RB (t->type_common.lang_flag_6);
>>>>>>> RB (t->type_common.typeless_storage);
>>>>>>> + RB (t->type_common.type_include_flexarray);
>>>>>>> }
>>>>>>>
>>>>>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
>>>>>>> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc
>>>>>>> index 1f3afcbbc86..efacdb7686f 100644
>>>>>>> --- a/gcc/print-tree.cc
>>>>>>> +++ b/gcc/print-tree.cc
>>>>>>> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
>>>>>>> && TYPE_CXX_ODR_P (node))
>>>>>>> fputs (" cxx-odr-p", file);
>>>>>>>
>>>>>>> + if ((code == RECORD_TYPE
>>>>>>> + || code == UNION_TYPE)
>>>>>>> + && TYPE_INCLUDE_FLEXARRAY (node))
>>>>>>> + fputs (" include-flexarray", file);
>>>>>>> +
>>>>>>> /* The transparent-union flag is used for different things in
>>>>>>> different nodes. */
>>>>>>> if ((code == UNION_TYPE || code == RECORD_TYPE)
>>>>>>> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>>>>> new file mode 100644
>>>>>>> index 00000000000..60078e11634
>>>>>>> --- /dev/null
>>>>>>> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
>>>>>>> @@ -0,0 +1,134 @@
>>>>>>> +/* PR 101832:
>>>>>>> + GCC extension accepts the case when a struct with a C99 flexible array
>>>>>>> + member is embedded into another struct (possibly recursively).
>>>>>>> + __builtin_object_size will treat such struct as flexible size.
>>>>>>> + However, when a structure with non-C99 flexible array member, i.e, trailing
>>>>>>> + [0], [1], or [4], is embedded into anther struct, the stucture will not
>>>>>>> + be treated as flexible size. */
>>>>>>> +/* { dg-do run } */
>>>>>>> +/* { dg-options "-O2" } */
>>>>>>> +
>>>>>>> +#include "builtin-object-size-common.h"
>>>>>>> +
>>>>>>> +#define expect(p, _v) do { \
>>>>>>> + size_t v = _v; \
>>>>>>> + if (p == v) \
>>>>>>> + __builtin_printf ("ok: %s == %zd\n", #p, p); \
>>>>>>> + else {\
>>>>>>> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
>>>>>>> + FAIL (); \
>>>>>>> + } \
>>>>>>> +} while (0);
>>>>>>> +
>>>>>>> +
>>>>>>> +struct A {
>>>>>>> + int n;
>>>>>>> + char data[];
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct B {
>>>>>>> + int m;
>>>>>>> + struct A a;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct C {
>>>>>>> + int q;
>>>>>>> + struct B b;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct A0 {
>>>>>>> + int n;
>>>>>>> + char data[0];
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct B0 {
>>>>>>> + int m;
>>>>>>> + struct A0 a;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct C0 {
>>>>>>> + int q;
>>>>>>> + struct B0 b;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct A1 {
>>>>>>> + int n;
>>>>>>> + char data[1];
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct B1 {
>>>>>>> + int m;
>>>>>>> + struct A1 a;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct C1 {
>>>>>>> + int q;
>>>>>>> + struct B1 b;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct An {
>>>>>>> + int n;
>>>>>>> + char data[8];
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct Bn {
>>>>>>> + int m;
>>>>>>> + struct An a;
>>>>>>> +};
>>>>>>> +
>>>>>>> +struct Cn {
>>>>>>> + int q;
>>>>>>> + struct Bn b;
>>>>>>> +};
>>>>>>> +
>>>>>>> +volatile void *magic1, *magic2;
>>>>>>> +
>>>>>>> +int main (int argc, char *argv[])
>>>>>>> +{
>>>>>>> + struct B *outer;
>>>>>>> + struct C *outest;
>>>>>>> +
>>>>>>> + /* Make sure optimization can't find some other object size. */
>>>>>>> + outer = (void *)magic1;
>>>>>>> + outest = (void *)magic2;
>>>>>>> +
>>>>>>> + expect (__builtin_object_size (&outer->a, 1), -1);
>>>>>>> + expect (__builtin_object_size (&outest->b, 1), -1);
>>>>>>> + expect (__builtin_object_size (&outest->b.a, 1), -1);
>>>>>>> +
>>>>>>> + struct B0 *outer0;
>>>>>>> + struct C0 *outest0;
>>>>>>> +
>>>>>>> + /* Make sure optimization can't find some other object size. */
>>>>>>> + outer0 = (void *)magic1;
>>>>>>> + outest0 = (void *)magic2;
>>>>>>> +
>>>>>>> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
>>>>>>> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
>>>>>>> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
>>>>>>> +
>>>>>>> + struct B1 *outer1;
>>>>>>> + struct C1 *outest1;
>>>>>>> +
>>>>>>> + /* Make sure optimization can't find some other object size. */
>>>>>>> + outer1 = (void *)magic1;
>>>>>>> + outest1 = (void *)magic2;
>>>>>>> +
>>>>>>> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
>>>>>>> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
>>>>>>> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
>>>>>>> +
>>>>>>> + struct Bn *outern;
>>>>>>> + struct Cn *outestn;
>>>>>>> +
>>>>>>> + /* Make sure optimization can't find some other object size. */
>>>>>>> + outern = (void *)magic1;
>>>>>>> + outestn = (void *)magic2;
>>>>>>> +
>>>>>>> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
>>>>>>> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
>>>>>>> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
>>>>>>> +
>>>>>>> + DONE ();
>>>>>>> + return 0;
>>>>>>> +}
>>>>>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
>>>>>>> index acd8deea34e..705d5702b9c 100644
>>>>>>> --- a/gcc/tree-core.h
>>>>>>> +++ b/gcc/tree-core.h
>>>>>>> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common {
>>>>>>> unsigned empty_flag : 1;
>>>>>>> unsigned indivisible_p : 1;
>>>>>>> RECORD_OR_UNION_CHECK
>>>>>>
>>>>>> it looks like the bit could be eventually shared with
>>>>>> no_named_args_stdarg_p (but its accessor doesn't use
>>>>>> FUNC_OR_METHOD_CHECK)
>>>>> You mean to let “type_include_flexarray” share the same bit with “no_named_args_stdarg_p” in order to save 1-bit space in IR?
>>>>> Then change the
>>>>>
>>>>> /* True if this is a stdarg function with no named arguments (C2x
>>>>> (...) prototype, where arguments can be accessed with va_start and
>>>>> va_arg), as opposed to an unprototyped function. */
>>>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>>>>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>>>>
>>>>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>>>>> at the last field recursively. */
>>>>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>>>>> (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>>>>>
>>>>> To:
>>>>>
>>>>> union {
>>>>> unsigned no_named_args_stdarg_p : 1;
>>>>> /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */
>>>>> unsigned int type_include_flexarray : 1;
>>>>> } shared_bit;
>>>>> unsigned spare : 15;
>>>>>
>>>>> /* True if this is a stdarg function with no named arguments (C2x
>>>>> (...) prototype, where arguments can be accessed with va_start and
>>>>> va_arg), as opposed to an unprototyped function. */
>>>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>>>>> (FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p)
>>>>>
>>>>> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>>>>> at the last field recursively. */
>>>>> #define TYPE_INCLUDE_FLEXARRAY(NODE) \
>>>>> (RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray)
>>>>
>>>> No, we're just overloading bits by using the same name for different
>>>> purposes. I don't think such a union would pack nicely. We overload
>>>> by documenting the uses, in the above cases the uses are separated
>>>> by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE
>>>> and the accessor macros should be updated to use the appropriate
>>>> tree check macros covering that so we don't try to inspect the
>>>> "wrong bit”.
>>> Okay, I see. Then should I change the name of that bit to one that reflect two usages?
>>>>
>>>>>>
>>>>>>> alias_set_type alias_set;
>>>>>>> tree pointer_to;
>>>>>>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
>>>>>>> index 9a936a91983..22b3c72ea6e 100644
>>>>>>> --- a/gcc/tree-object-size.cc
>>>>>>> +++ b/gcc/tree-object-size.cc
>>>>>>> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
>>>>>>> v = NULL_TREE;
>>>>>>> break;
>>>>>>> case COMPONENT_REF:
>>>>>>> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>>>>>>> + /* When the ref is not to an array, a record or a union, it
>>>>>>> + will not have flexible size, compute the object size
>>>>>>> + directly. */
>>>>>>> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
>>>>>>> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE)
>>>>>>> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE))
>>>>>>> {
>>>>>>> v = NULL_TREE;
>>>>>>> break;
>>>>>>> }
>>>>>>> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
>>>>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> - != UNION_TYPE
>>>>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> - != QUAL_UNION_TYPE)
>>>>>>> - break;
>>>>>>> - else
>>>>>>> - v = TREE_OPERAND (v, 0);
>>>>>>> - if (TREE_CODE (v) == COMPONENT_REF
>>>>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> - == RECORD_TYPE)
>>>>>>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE
>>>>>>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE)
>>>>>>> + /* if the record or union does not include a flexible array
>>>>>>> + recursively, compute the object size directly. */
>>>>>>> {
>>>>>>> - /* compute object size only if v is not a
>>>>>>> - flexible array member. */
>>>>>>> - if (!is_flexible_array_mem_ref)
>>>>>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v)))
>>>>>>> {
>>>>>>> v = NULL_TREE;
>>>>>>> break;
>>>>>>> }
>>>>>>> - v = TREE_OPERAND (v, 0);
>>>>>>> + else
>>>>>>> + v = TREE_OPERAND (v, 0);
>>>>>>> }
>>>>>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> - != UNION_TYPE
>>>>>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> - != QUAL_UNION_TYPE)
>>>>>>> - break;
>>>>>>> - else
>>>>>>> - v = TREE_OPERAND (v, 0);
>>>>>>> - if (v != pt_var)
>>>>>>> - v = NULL_TREE;
>>>>>>> else
>>>>>>> - v = pt_var;
>>>>>>> + {
>>>>>>> + /* Now the ref is to an array type. */
>>>>>>> + is_flexible_array_mem_ref
>>>>>>> + = array_ref_flexible_size_p (v);
>>>>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> + != UNION_TYPE
>>>>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> + != QUAL_UNION_TYPE)
>>>>>>> + break;
>>>>>>> + else
>>>>>>> + v = TREE_OPERAND (v, 0);
>>>>>>> + if (TREE_CODE (v) == COMPONENT_REF
>>>>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> + == RECORD_TYPE)
>>>>>>> + {
>>>>>>> + /* compute object size only if v is not a
>>>>>>> + flexible array member. */
>>>>>>> + if (!is_flexible_array_mem_ref)
>>>>>>> + {
>>>>>>> + v = NULL_TREE;
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + v = TREE_OPERAND (v, 0);
>>>>>>> + }
>>>>>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>>>>>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> + != UNION_TYPE
>>>>>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
>>>>>>> + != QUAL_UNION_TYPE)
>>>>>>> + break;
>>>>>>> + else
>>>>>>> + v = TREE_OPERAND (v, 0);
>>>>>>> + if (v != pt_var)
>>>>>>> + v = NULL_TREE;
>>>>>>> + else
>>>>>>> + v = pt_var;
>>>>>>> + }
>>>>>>> break;
>>>>>>> default:
>>>>>>> v = pt_var;
>>>>>>> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc
>>>>>>> index d4dc30f048f..c19ede0631d 100644
>>>>>>> --- a/gcc/tree-streamer-in.cc
>>>>>>> +++ b/gcc/tree-streamer-in.cc
>>>>>>> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>>>>>>> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>>>> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>>>> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>>>> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>>>> }
>>>>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>>>>>>> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1);
>>>>>>> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc
>>>>>>> index d107229da5c..73e4b4e547c 100644
>>>>>>> --- a/gcc/tree-streamer-out.cc
>>>>>>> +++ b/gcc/tree-streamer-out.cc
>>>>>>> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>>>>>>> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr)
>>>>>>> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr))
>>>>>>> : TYPE_CXX_ODR_P (expr), 1);
>>>>>>> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1);
>>>>>>> }
>>>>>>> else if (TREE_CODE (expr) == ARRAY_TYPE)
>>>>>>> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1);
>>>>>>> diff --git a/gcc/tree.h b/gcc/tree.h
>>>>>>> index 92ac0e6a214..ab1cdc3dc85 100644
>>>>>>> --- a/gcc/tree.h
>>>>>>> +++ b/gcc/tree.h
>>>>>>> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
>>>>>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
>>>>>>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
>>>>>>>
>>>>>>> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
>>>>>>> + at the last field recursively. */
>>>>>>> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \
>>>>>>> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray)
>>>>>>
>>>>>> Please use RECORD_OR_UNION_CHECK.
>>>>> Okay.
>>>>>> The comment "at the last field"
>>>>>> doesn't seem to match implementation? (at least not obviously)
>>>>> The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the last field. (And recursively).
>>>>>> Given this is a generic header expanding on what a "flexible array member"
>>>>>> is to the middle-end here would be good. Especially the guarantees
>>>>>> made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY
>>>>>> vs. DECL_FLEXARRAY).
>>>>>
>>>>> The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex array by default.
>>>>> It’s only set to TRUE when the struct/union includes a flex array at the last field (and recursively). So, I think it should be correct.
>>>>> Let me know if I missed anything here.
>>>>
>>>> See above - I was looking at the use (but I'm not familiar with that
>>>> code), and wondered how the change affects uses from other frontends.
>>>
>>> Please see my explanation above, I think the default behavior from other FE should be kept correctly with this patch.
>>>
>>> thanks.
>>>
>>> Qing
>>>>
>>>> Richard.
>>
>>
>
> --
> 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)
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [V4][PATCH 2/2] Update documentation to clarify a GCC extension
2023-02-24 18:35 ` [V4][PATCH 2/2] Update documentation to clarify a GCC extension Qing Zhao
2023-03-03 0:03 ` Qing Zhao
@ 2023-03-15 3:26 ` Sandra Loosemore
2023-03-15 14:42 ` Qing Zhao
1 sibling, 1 reply; 18+ messages in thread
From: Sandra Loosemore @ 2023-03-15 3:26 UTC (permalink / raw)
To: Qing Zhao, joseph, rguenther; +Cc: siddhesh, keescook, gcc-patches
On 2/24/23 11:35, Qing Zhao via Gcc-patches wrote:
> gcc/c-family/ChangeLog:
>
> * c.opt: New option -Wgnu-variable-sized-type-not-at-end.
>
> gcc/c/ChangeLog:
>
> * c-decl.cc (finish_struct): Issue warnings for new option.
>
> gcc/ChangeLog:
>
> * doc/extend.texi: Document GCC extension on a structure containing
> a flexible array member to be a member of another structure.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/variable-sized-type-flex-array.c: New test.
I'm only a documentation (and nios2) maintainer so I cannot approve
adding a new option or warning. I was going to review the documentation
parts, at least, but I think this proposal is technically flawed because
it is trying to document something that is undefined behavior in ways
that it doesn't actually behave on all targets.
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index c1122916255..e278148c332 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -1748,7 +1748,53 @@ Flexible array members may only appear as the last member of a
> A structure containing a flexible array member, or a union containing
> such a structure (possibly recursively), may not be a member of a
> structure or an element of an array. (However, these uses are
> -permitted by GCC as extensions.)
> +permitted by GCC as extensions, see details below.)
> +@end itemize
> +
> +GCC extension accepts a structure containing an ISO C99 @dfn{flexible array
> +member}, or a union containing such a structure (possibly recursively)
> +to be a member of a structure.
> +
> +There are two situations:
> +
> +@itemize @bullet
> +@item
> +The structure with a C99 flexible array member is the last field of another
> +structure, for example:
> +
> +@smallexample
> +struct flex @{ int length; char data[]; @};
> +union union_flex @{ int others; struct flex f; @};
> +
> +struct out_flex_struct @{ int m; struct flex flex_data; @};
> +struct out_flex_union @{ int n; union union_flex flex_data; @};
> +@end smallexample
> +
> +In the above, both @code{out_flex_struct.flex_data.data[]} and
> +@code{out_flex_union.flex_data.f.data[]} are considered as flexible arrays too.
> +
> +
> +@item
> +The structure with a C99 flexible array member is the middle field of another
> +structure, for example:
> +
> +@smallexample
> +struct flex @{ int length; char data[]; @};
> +
> +struct mid_flex @{ int m; struct flex flex_data; int n; @};
> +@end smallexample
> +
> +In the above, @code{mid_flex.flex_data.data[]} is allowed to be extended
> +flexibly to the padding. E.g, up to 4 elements.
I think this paragraph is incorrect; how GCC lays out this structure
depends on the target and ABI. Looking at output from a GCC 12
nios2-elf build I have handy, I see it is in fact laying out mid_flex so
that member n is at the same address at offset 8 as flex_data.data[0],
which is not useful at all.
> +However, relying on space in struct padding is a bad programming practice,
> +compilers do not handle such extension consistently, Any code relying on
> +this behavior should be modified to ensure that flexible array members
> +only end up at the ends of structures.
> +
> +Please use warning option @option{-Wgnu-variable-sized-type-not-at-end} to
> +identify all such cases in the source code and modify them. This extension
> +will be deprecated from gcc in the next release.
My suggestion would be to make this a hard error instead of a warning,
unless there is some real body of code out there that depends on this
feature on a target that actually does insert padding. If it's a
warning, it ought to be enabled by default. And, rather than trying to
document the behavior, the manual should just say it's undefined.
-Sandra
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [V4][PATCH 2/2] Update documentation to clarify a GCC extension
2023-03-15 3:26 ` Sandra Loosemore
@ 2023-03-15 14:42 ` Qing Zhao
0 siblings, 0 replies; 18+ messages in thread
From: Qing Zhao @ 2023-03-15 14:42 UTC (permalink / raw)
To: Sandra Loosemore
Cc: Joseph Myers, Richard Biener, Siddhesh Poyarekar, keescook, gcc-patches
Hi, Sandra,
Thanks a lot for your review and comment.
Yes, the issue you raised in below was a really tough one that I didn’t feel very comfortable to handle it well…
This documentation change is mainly to fix: PR77650 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77650).
The real user case for it was the old glibc headers. (As Joshph mentioned in the comment #1:
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77650#c1)
And from my understanding, the code in glibc has already been fixed.
https://gcc.gnu.org/pipermail/gcc-patches/2023-February/611220.html
But I am not sure in addition to Glibc, whether there are other use cases out there currently.
That’s the reason we cannot simply reject this case as an error at this moment. And the new warning -Wgnu-variable-sized-type-not-at-end
Is added to catch such case in user case to encourage user to update them. And then later we can completely delete this case from GCC support.
Right now, GCC’s implementation cannot handle such case consistently. So, how to document this case is really hard.
> On Mar 14, 2023, at 11:26 PM, Sandra Loosemore <sandra@codesourcery.com> wrote:
>
> On 2/24/23 11:35, Qing Zhao via Gcc-patches wrote:
>
>> gcc/c-family/ChangeLog:
>> * c.opt: New option -Wgnu-variable-sized-type-not-at-end.
>> gcc/c/ChangeLog:
>> * c-decl.cc (finish_struct): Issue warnings for new option.
>> gcc/ChangeLog:
>> * doc/extend.texi: Document GCC extension on a structure containing
>> a flexible array member to be a member of another structure.
>> gcc/testsuite/ChangeLog:
>> * gcc.dg/variable-sized-type-flex-array.c: New test.
>
> I'm only a documentation (and nios2) maintainer so I cannot approve adding a new option or warning. I was going to review the documentation parts, at least, but I think this proposal is technically flawed because it is trying to document something that is undefined behavior in ways that it doesn't actually behave on all targets.
So, should we mention it in the documentation at all?
Of mention it but specify the possible undefined behavior, the potential risk to use this case, and then warn the user to get rid of this usage with the new warning?
>
>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>> index c1122916255..e278148c332 100644
>> --- a/gcc/doc/extend.texi
>> +++ b/gcc/doc/extend.texi
>> @@ -1748,7 +1748,53 @@ Flexible array members may only appear as the last member of a
>> A structure containing a flexible array member, or a union containing
>> such a structure (possibly recursively), may not be a member of a
>> structure or an element of an array. (However, these uses are
>> -permitted by GCC as extensions.)
>> +permitted by GCC as extensions, see details below.)
>> +@end itemize
>> +
>> +GCC extension accepts a structure containing an ISO C99 @dfn{flexible array
>> +member}, or a union containing such a structure (possibly recursively)
>> +to be a member of a structure.
>> +
>> +There are two situations:
>> +
>> +@itemize @bullet
>> +@item
>> +The structure with a C99 flexible array member is the last field of another
>> +structure, for example:
>> +
>> +@smallexample
>> +struct flex @{ int length; char data[]; @};
>> +union union_flex @{ int others; struct flex f; @};
>> +
>> +struct out_flex_struct @{ int m; struct flex flex_data; @};
>> +struct out_flex_union @{ int n; union union_flex flex_data; @};
>> +@end smallexample
>> +
>> +In the above, both @code{out_flex_struct.flex_data.data[]} and
>> +@code{out_flex_union.flex_data.f.data[]} are considered as flexible arrays too.
>> +
>> +
>> +@item
>> +The structure with a C99 flexible array member is the middle field of another
>> +structure, for example:
>> +
>> +@smallexample
>> +struct flex @{ int length; char data[]; @};
>> +
>> +struct mid_flex @{ int m; struct flex flex_data; int n; @};
>> +@end smallexample
>> +
>> +In the above, @code{mid_flex.flex_data.data[]} is allowed to be extended
>> +flexibly to the padding. E.g, up to 4 elements.
>
> I think this paragraph is incorrect; how GCC lays out this structure depends on the target and ABI. Looking at output from a GCC 12 nios2-elf build I have handy, I see it is in fact laying out mid_flex so that member n is at the same address at offset 8 as flex_data.data[0], which is not useful at all.
I think that this behavior you mentioned is consistent with what’s the expected. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77650#c5.
But not sure whether we should document it or not?
>> +However, relying on space in struct padding is a bad programming practice,
>> +compilers do not handle such extension consistently, Any code relying on
>> +this behavior should be modified to ensure that flexible array members
>> +only end up at the ends of structures.
>> +
>> +Please use warning option @option{-Wgnu-variable-sized-type-not-at-end} to
>> +identify all such cases in the source code and modify them. This extension
>> +will be deprecated from gcc in the next release.
>
> My suggestion would be to make this a hard error instead of a warning, unless there is some real body of code out there that depends on this feature on a target that actually does insert padding. If it's a warning, it ought to be enabled by default. And, rather than trying to document the behavior, the manual should just say it's undefined.
As I mentioned above, the warning was added mainly for unknown user cases, to warn them about this case and encourage them to update the code to prepare GCC deprecating this misfeature.
Yes, I think “rather than trying to document the behavior, just say it’s undefined” might be more reasonable, I will update the doc based on this suggestion.
Thanks a lot.
Qing
>
> -Sandra
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-03-15 14:42 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-24 18:35 [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-02-24 18:35 ` [v4][PATCH 1/2] Handle component_ref to a structre/union field including C99 FAM [PR101832] Qing Zhao
2023-03-03 0:03 ` Qing Zhao
2023-03-09 12:20 ` Richard Biener
2023-03-09 16:11 ` Qing Zhao
2023-03-10 7:54 ` Richard Biener
2023-03-10 13:48 ` Qing Zhao
2023-03-13 22:42 ` Qing Zhao
2023-03-14 9:04 ` Richard Biener
2023-03-14 13:09 ` Qing Zhao
2023-02-24 18:35 ` [V4][PATCH 2/2] Update documentation to clarify a GCC extension Qing Zhao
2023-03-03 0:03 ` Qing Zhao
2023-03-12 23:14 ` Sandra Loosemore
2023-03-13 12:46 ` Qing Zhao
2023-03-15 3:26 ` Sandra Loosemore
2023-03-15 14:42 ` Qing Zhao
2023-03-03 0:02 ` [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-03-08 18:45 ` Kees Cook
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).