[GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

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

Hi, 

Per our discussion in the bug report, I came up with the following patch:

=======

PR101836: Add a new option -fstrict-flex-array[=n]

Add the new option and use it in __builtin_object_size.

Treat the trailing array of a structure as a flexible array member in a
stricter way.  The value of 'n' controls the level of strictness.
'n'=0 is the least strict, all trailing arrays of structures are treated
as flexible array members; This is the default behavior of GCC without specify
this option.
'n'=3 is the strictest, only when the trailing array is declared as a
flexible array member per C99 standard onwards ([]), it is treated as a
flexible array member;
There are two more levels in between 0 and 3, which are provided to support
older codes that use GCC zero-length array extension ([0]), or one-size array as
flexible array member ([1]):
When 'n' is 1, the trailing array is treated as a flexible array member
when it is declared as either [], [0], or [1];
When 'n' is 2, the trailing array is treated as a flexible array member
when it is declared as either [], or [0].

There are other places in GCC that conservatively treat flexible array members.
A follow-up patch will make -ftrict-flex-array option to control all these
places consistently.

Bootstrapped and regression tested on both X86 and aarch64, no issues.

Any comment and suggestion?

Okay for commit to Gcc13?

thanks.

Qing

=======================

gcc/

       PR tree-optimization/101836
       * common.opt (fstrict-flex-array, fstrict-flex-array=): New options.
       * doc/invoke.texi (-fstrict-flex-array, -fstrict-flex-array=): Document.
       * tree-object-size.cc (addr_object_size): Call is_flexible_array_p to
       check whether an array is a flexible array.
       * tree.cc (special_array_member_type): New routine.
       (is_flexible_array_p): New routine.
       (component_ref_size): Call special_array_member_type to decide the
       type of special array member.
       * tree.h (enum struct special_array_member): Add is_vla, trail_flex.
       (special_array_member_type): New prototype.
       (is_flexible_array_p): New prototype.

gcc/testsuite/

       PR tree-optimization/101836
       * gcc.dg/pr101836.c: New test.
       * gcc.dg/pr101836_1.c: New test.
       * gcc.dg/pr101836_2.c: New test.
       * gcc.dg/pr101836_3.c: New test.
       * gcc.dg/pr101836_4.c: New test.
       * gcc.dg/pr101836_5.c: New test.


The complete patch is:


[-- Attachment #2: 0001-PR101836-Add-a-new-option-fstrict-flex-array-n.patch --]
[-- Type: application/octet-stream, Size: 26062 bytes --]

From 880f1024c5d1b8d118ff3c84d3da43aebc0ad27a Mon Sep 17 00:00:00 2001
From: Qing Zhao <qing.zhao@oracle.com>
Date: Thu, 16 Jun 2022 21:05:39 +0000
Subject: [PATCH] PR101836: Add a new option -fstrict-flex-array[=n]

Add the new option and use it in __builtin_object_size.

Treat the trailing array of a structure as a flexible array member in a
stricter way.  The value of 'n' controls the level of strictness.
'n'=0 is the least strict, all trailing arrays of structures are treated
as flexible array members; This is the default behavior of GCC without specify
this option.
'n'=3 is the strictest, only when the trailing array is declared as a
flexible array member per C99 standard onwards ([]), it is treated as a
flexible array member;
There are two more levels in between 0 and 3, which are provided to support
older codes that use GCC zero-length array extension ([0]), or one-size array as
flexible array member ([1]):
When 'n' is 1, the trailing array is treated as a flexible array member
when it is declared as either [], [0], or [1];
When 'n' is 2, the trailing array is treated as a flexible array member
when it is declared as either [], or [0].

There are other places in GCC that conservatively treat flexible array members.
A follow-up patch will make -ftrict-flex-array option to control all these
places consistently.

gcc/

	PR tree-optimization/101836
	* common.opt (fstrict-flex-array, fstrict-flex-array=): New options.
	* doc/invoke.texi (-fstrict-flex-array, -fstrict-flex-array=): Document.
	* tree-object-size.cc (addr_object_size): Call is_flexible_array_p to
	check whether an array is a flexible array.
	* tree.cc (special_array_member_type): New routine.
	(is_flexible_array_p): New routine.
	(component_ref_size): Call special_array_member_type to decide the
	type of special array member.
	* tree.h (enum struct special_array_member): Add is_vla, trail_flex.
	(special_array_member_type): New prototype.
	(is_flexible_array_p): New prototype.

gcc/testsuite/

	PR tree-optimization/101836
	* gcc.dg/pr101836.c: New test.
	* gcc.dg/pr101836_1.c: New test.
	* gcc.dg/pr101836_2.c: New test.
	* gcc.dg/pr101836_3.c: New test.
	* gcc.dg/pr101836_4.c: New test.
	* gcc.dg/pr101836_5.c: New test.
---
 gcc/common.opt                    |   8 ++
 gcc/doc/invoke.texi               |  33 ++++++-
 gcc/testsuite/gcc.dg/pr101836.c   |  60 +++++++++++
 gcc/testsuite/gcc.dg/pr101836_1.c |  60 +++++++++++
 gcc/testsuite/gcc.dg/pr101836_2.c |  60 +++++++++++
 gcc/testsuite/gcc.dg/pr101836_3.c |  60 +++++++++++
 gcc/testsuite/gcc.dg/pr101836_4.c |  60 +++++++++++
 gcc/testsuite/gcc.dg/pr101836_5.c |  60 +++++++++++
 gcc/tree-object-size.cc           |  16 ++-
 gcc/tree.cc                       | 159 ++++++++++++++++++++++--------
 gcc/tree.h                        |  16 ++-
 11 files changed, 536 insertions(+), 56 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr101836.c
 create mode 100644 gcc/testsuite/gcc.dg/pr101836_1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr101836_2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr101836_3.c
 create mode 100644 gcc/testsuite/gcc.dg/pr101836_4.c
 create mode 100644 gcc/testsuite/gcc.dg/pr101836_5.c

diff --git a/gcc/common.opt b/gcc/common.opt
index 32917aafcae..e28697ab8b1 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2772,6 +2772,14 @@ fstrict-aliasing
 Common Var(flag_strict_aliasing) Optimization
 Assume strict aliasing rules apply.
 
+fstrict-flex-array
+Common Alias(fstrict-flex-array=,3,0)
+
+fstrict-flex-array=
+Common Joined RejectNegative UInteger Var(flag_strict_flex_array) Init(0) IntegerRange(0,3) Optimization
+-fstrict-flex-array=<level>    Treat the trailing array of a structure as a flexible array in a stricter way.  The default is treating all trailing arrays of structures as flexible arrays.
+
+
 fstrict-overflow
 Common
 Treat signed overflow as undefined.  Negated as -fwrapv -fwrapv-pointer.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 81d13f4e78e..7a72e29cedf 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -579,7 +579,7 @@ Objective-C and Objective-C++ Dialects}.
 -fsplit-paths @gol
 -fsplit-wide-types  -fsplit-wide-types-early  -fssa-backprop  -fssa-phiopt @gol
 -fstdarg-opt  -fstore-merging  -fstrict-aliasing -fipa-strict-aliasing @gol
--fthread-jumps  -ftracer  -ftree-bit-ccp @gol
+-fstrict-flex-array[=@var{n}] -fthread-jumps  -ftracer  -ftree-bit-ccp @gol
 -ftree-builtin-call-dce  -ftree-ccp  -ftree-ch @gol
 -ftree-coalesce-vars  -ftree-copy-prop  -ftree-dce  -ftree-dominator-opts @gol
 -ftree-dse  -ftree-forwprop  -ftree-fre  -fcode-hoisting @gol
@@ -12787,6 +12787,37 @@ function boundary.
 The @option{-fipa-strict-aliasing} option is enabled by default and is
 effective only in combination with @option{-fstrict-aliasing}.
 
+@item -fstrict-flex-array
+@opindex fstrict-flex-array
+@opindex fno-strict-flex-array
+Treat the trailing array of a structure as a flexible array member in a
+stricter way.
+The positive form is equivalent to @option{-fstrict-flex-array=3}, which is the
+strictest.  A trailing array is treated as a flexible array member only when it
+is declared as a flexible array member per C99 standard onwards.
+The negative form is equivalent to @option{-fstrict-flex-array=0}, which is the
+least strict.  All trailing arrays of structures are treated as flexible array
+members.
+
+@item -fstrict-flex-array=@var{n}
+@opindex fstrict-flex-array=@var{n}
+Treat the trailing array of a structure as a flexible array member in a
+stricter way.  The value of @var{n} controls the level of strictness.
+@var{n} is 0 is the least strict, all trailing arrays of structures are treated
+as flexible array members; This is the default behavior of GCC without specify
+this option.
+@var{n} is 3 is the strictest, only when the trailing array is declared as a
+flexible array member per C99 standard onwards ([]), it is treated as a
+flexible array member;
+There are two more levels in between 0 and 3, which are provided to support
+older codes that use GCC zero-length array extension ([0]), or one-size array as
+flexible array member ([1]):
+When @var{n} is 1, the trailing array is treated as a flexible array member
+when it is declared as either [], [0], or [1];
+When @var{n} is 2, the trailing array is treated as a flexible array member
+when it is declared as either [], or [0].
+
+
 @item -falign-functions
 @itemx -falign-functions=@var{n}
 @itemx -falign-functions=@var{n}:@var{m}
diff --git a/gcc/testsuite/gcc.dg/pr101836.c b/gcc/testsuite/gcc.dg/pr101836.c
new file mode 100644
index 00000000000..e5b4e5160a4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101836.c
@@ -0,0 +1,60 @@
+/* -fstrict-flex-array is aliased with -ftrict-flex-array=3, which is the
+   strictest, only [] is treated as flexible array.  */ 
+/* PR tree-optimization/101836 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fstrict-flex-array" } */
+
+#include <stdio.h>
+
+#define expect(p, _v) do { \
+    size_t v = _v; \
+    if (p == v) \
+        printf("ok:  %s == %zd\n", #p, p); \
+    else \
+	{  \
+          printf("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+	  __builtin_abort (); \
+	} \
+} while (0);
+
+struct trailing_array_1 {
+    int a;
+    int b;
+    int c[4];
+};
+
+struct trailing_array_2 {
+    int a;
+    int b;
+    int c[1];
+};
+
+struct trailing_array_3 {
+    int a;
+    int b;
+    int c[0];
+};
+struct trailing_array_4 {
+    int a;
+    int b;
+    int c[];
+};
+
+void __attribute__((__noinline__)) stuff(
+    struct trailing_array_1 *normal,
+    struct trailing_array_2 *trailing_1,
+    struct trailing_array_3 *trailing_0,
+    struct trailing_array_4 *trailing_flex)
+{
+    expect(__builtin_object_size(normal->c, 1), 16);
+    expect(__builtin_object_size(trailing_1->c, 1), 4);
+    expect(__builtin_object_size(trailing_0->c, 1), 0);
+    expect(__builtin_object_size(trailing_flex->c, 1), -1);
+}
+
+int main(int argc, char *argv[])
+{
+    stuff((void *)argv[0], (void *)argv[0], (void *)argv[0], (void *)argv[0]);
+
+    return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr101836_1.c b/gcc/testsuite/gcc.dg/pr101836_1.c
new file mode 100644
index 00000000000..30ea20427a5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101836_1.c
@@ -0,0 +1,60 @@
+/* -fstrict-flex-array=3 is the strictest, only [] is treated as
+   flexible array.  */ 
+/* PR tree-optimization/101836 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fstrict-flex-array=3" } */
+
+#include <stdio.h>
+
+#define expect(p, _v) do { \
+    size_t v = _v; \
+    if (p == v) \
+        printf("ok:  %s == %zd\n", #p, p); \
+    else \
+	{  \
+          printf("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+	  __builtin_abort (); \
+	} \
+} while (0);
+
+struct trailing_array_1 {
+    int a;
+    int b;
+    int c[4];
+};
+
+struct trailing_array_2 {
+    int a;
+    int b;
+    int c[1];
+};
+
+struct trailing_array_3 {
+    int a;
+    int b;
+    int c[0];
+};
+struct trailing_array_4 {
+    int a;
+    int b;
+    int c[];
+};
+
+void __attribute__((__noinline__)) stuff(
+    struct trailing_array_1 *normal,
+    struct trailing_array_2 *trailing_1,
+    struct trailing_array_3 *trailing_0,
+    struct trailing_array_4 *trailing_flex)
+{
+    expect(__builtin_object_size(normal->c, 1), 16);
+    expect(__builtin_object_size(trailing_1->c, 1), 4);
+    expect(__builtin_object_size(trailing_0->c, 1), 0);
+    expect(__builtin_object_size(trailing_flex->c, 1), -1);
+}
+
+int main(int argc, char *argv[])
+{
+    stuff((void *)argv[0], (void *)argv[0], (void *)argv[0], (void *)argv[0]);
+
+    return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr101836_2.c b/gcc/testsuite/gcc.dg/pr101836_2.c
new file mode 100644
index 00000000000..ebbe88f433c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101836_2.c
@@ -0,0 +1,60 @@
+/* When -fstrict-flex-array=2, only [] and [0] are treated as flexiable
+   arrays.  */
+/* PR tree-optimization/101836 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fstrict-flex-array=2" } */
+
+#include <stdio.h>
+
+#define expect(p, _v) do { \
+    size_t v = _v; \
+    if (p == v) \
+        printf("ok:  %s == %zd\n", #p, p); \
+    else \
+	{  \
+          printf("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+	  __builtin_abort (); \
+	} \
+} while (0);
+
+struct trailing_array_1 {
+    int a;
+    int b;
+    int c[4];
+};
+
+struct trailing_array_2 {
+    int a;
+    int b;
+    int c[1];
+};
+
+struct trailing_array_3 {
+    int a;
+    int b;
+    int c[0];
+};
+struct trailing_array_4 {
+    int a;
+    int b;
+    int c[];
+};
+
+void __attribute__((__noinline__)) stuff(
+    struct trailing_array_1 *normal,
+    struct trailing_array_2 *trailing_1,
+    struct trailing_array_3 *trailing_0,
+    struct trailing_array_4 *trailing_flex)
+{
+    expect(__builtin_object_size(normal->c, 1), 16);
+    expect(__builtin_object_size(trailing_1->c, 1), 4);
+    expect(__builtin_object_size(trailing_0->c, 1), -1);
+    expect(__builtin_object_size(trailing_flex->c, 1), -1);
+}
+
+int main(int argc, char *argv[])
+{
+    stuff((void *)argv[0], (void *)argv[0], (void *)argv[0], (void *)argv[0]);
+
+    return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr101836_3.c b/gcc/testsuite/gcc.dg/pr101836_3.c
new file mode 100644
index 00000000000..d4ba0afe5bc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101836_3.c
@@ -0,0 +1,60 @@
+/* When -fstrict-flex-array=1, [], [0], and [1] are treated as flexible
+   arrays.  */
+/* PR tree-optimization/101836 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fstrict-flex-array=1" } */
+
+#include <stdio.h>
+
+#define expect(p, _v) do { \
+    size_t v = _v; \
+    if (p == v) \
+        printf("ok:  %s == %zd\n", #p, p); \
+    else \
+	{  \
+          printf("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+	  __builtin_abort (); \
+	} \
+} while (0);
+
+struct trailing_array_1 {
+    int a;
+    int b;
+    int c[4];
+};
+
+struct trailing_array_2 {
+    int a;
+    int b;
+    int c[1];
+};
+
+struct trailing_array_3 {
+    int a;
+    int b;
+    int c[0];
+};
+struct trailing_array_4 {
+    int a;
+    int b;
+    int c[];
+};
+
+void __attribute__((__noinline__)) stuff(
+    struct trailing_array_1 *normal,
+    struct trailing_array_2 *trailing_1,
+    struct trailing_array_3 *trailing_0,
+    struct trailing_array_4 *trailing_flex)
+{
+    expect(__builtin_object_size(normal->c, 1), 16);
+    expect(__builtin_object_size(trailing_1->c, 1), -1);
+    expect(__builtin_object_size(trailing_0->c, 1), -1);
+    expect(__builtin_object_size(trailing_flex->c, 1), -1);
+}
+
+int main(int argc, char *argv[])
+{
+    stuff((void *)argv[0], (void *)argv[0], (void *)argv[0], (void *)argv[0]);
+
+    return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr101836_4.c b/gcc/testsuite/gcc.dg/pr101836_4.c
new file mode 100644
index 00000000000..b10d3ce312d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101836_4.c
@@ -0,0 +1,60 @@
+/* when -fstrict-flex-array=0, all trailing arrays are treated as
+   flexible arrays.  */
+/* PR tree-optimization/101836 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fstrict-flex-array=0" } */
+
+#include <stdio.h>
+
+#define expect(p, _v) do { \
+    size_t v = _v; \
+    if (p == v) \
+        printf("ok:  %s == %zd\n", #p, p); \
+    else \
+	{  \
+          printf("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+	  __builtin_abort (); \
+	} \
+} while (0);
+
+struct trailing_array_1 {
+    int a;
+    int b;
+    int c[4];
+};
+
+struct trailing_array_2 {
+    int a;
+    int b;
+    int c[1];
+};
+
+struct trailing_array_3 {
+    int a;
+    int b;
+    int c[0];
+};
+struct trailing_array_4 {
+    int a;
+    int b;
+    int c[];
+};
+
+void __attribute__((__noinline__)) stuff(
+    struct trailing_array_1 *normal,
+    struct trailing_array_2 *trailing_1,
+    struct trailing_array_3 *trailing_0,
+    struct trailing_array_4 *trailing_flex)
+{
+    expect(__builtin_object_size(normal->c, 1), -1);
+    expect(__builtin_object_size(trailing_1->c, 1), -1);
+    expect(__builtin_object_size(trailing_0->c, 1), -1);
+    expect(__builtin_object_size(trailing_flex->c, 1), -1);
+}
+
+int main(int argc, char *argv[])
+{
+    stuff((void *)argv[0], (void *)argv[0], (void *)argv[0], (void *)argv[0]);
+
+    return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/pr101836_5.c b/gcc/testsuite/gcc.dg/pr101836_5.c
new file mode 100644
index 00000000000..2f6b5f7ae1f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101836_5.c
@@ -0,0 +1,60 @@
+/* -fno-strict-flex-array is aliased to -fstrict-flex-array=0,
+   all trailing arrays are treated as flexible array.  */
+/* PR tree-optimization/101836 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-strict-flex-array" } */
+
+#include <stdio.h>
+
+#define expect(p, _v) do { \
+    size_t v = _v; \
+    if (p == v) \
+        printf("ok:  %s == %zd\n", #p, p); \
+    else \
+	{  \
+          printf("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
+	  __builtin_abort (); \
+	} \
+} while (0);
+
+struct trailing_array_1 {
+    int a;
+    int b;
+    int c[4];
+};
+
+struct trailing_array_2 {
+    int a;
+    int b;
+    int c[1];
+};
+
+struct trailing_array_3 {
+    int a;
+    int b;
+    int c[0];
+};
+struct trailing_array_4 {
+    int a;
+    int b;
+    int c[];
+};
+
+void __attribute__((__noinline__)) stuff(
+    struct trailing_array_1 *normal,
+    struct trailing_array_2 *trailing_1,
+    struct trailing_array_3 *trailing_0,
+    struct trailing_array_4 *trailing_flex)
+{
+    expect(__builtin_object_size(normal->c, 1), -1);
+    expect(__builtin_object_size(trailing_1->c, 1), -1);
+    expect(__builtin_object_size(trailing_0->c, 1), -1);
+    expect(__builtin_object_size(trailing_flex->c, 1), -1);
+}
+
+int main(int argc, char *argv[])
+{
+    stuff((void *)argv[0], (void *)argv[0], (void *)argv[0], (void *)argv[0]);
+
+    return 0;
+}
diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc
index 12bc0868b77..9f6e78640ed 100644
--- a/gcc/tree-object-size.cc
+++ b/gcc/tree-object-size.cc
@@ -604,9 +604,9 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
 	  else if (var != pt_var && TREE_CODE (pt_var) == MEM_REF)
 	    {
 	      tree v = var;
-	      /* For &X->fld, compute object size only if fld isn't the last
-		 field, as struct { int i; char c[1]; } is often used instead
-		 of flexible array member.  */
+	      bool is_flexible_array = false;
+	      /* For &X->fld, compute object size if fld isn't a flexible array
+		 member.  */
 	      while (v && v != pt_var)
 		switch (TREE_CODE (v))
 		  {
@@ -628,6 +628,7 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
 		    v = NULL_TREE;
 		    break;
 		  case COMPONENT_REF:
+		    is_flexible_array = is_flexible_array_p (v);
 		    if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
 		      {
 			v = NULL_TREE;
@@ -645,12 +646,9 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
 			&& TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
 			   == RECORD_TYPE)
 		      {
-			tree fld_chain = DECL_CHAIN (TREE_OPERAND (v, 1));
-			for (; fld_chain; fld_chain = DECL_CHAIN (fld_chain))
-			  if (TREE_CODE (fld_chain) == FIELD_DECL)
-			    break;
-
-			if (fld_chain)
+			/* compute object size only if v is not a
+			   flexible array member.  */
+			if (!is_flexible_array)
 			  {
 			    v = NULL_TREE;
 			    break;
diff --git a/gcc/tree.cc b/gcc/tree.cc
index 2bfb67489c6..329684b57df 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -12762,6 +12762,112 @@ array_at_struct_end_p (tree ref)
   return true;
 }
 
+/* Return the type of special_array_member for the COMPONENT_REF REF.  */
+special_array_member
+special_array_member_type (tree ref)
+{
+  gcc_assert (TREE_CODE (ref) == COMPONENT_REF);
+  bool trailing = array_at_struct_end_p (ref);
+  special_array_member sam = special_array_member::none;
+
+  /* if this reference is a trailing array, and has a C99 flexible array
+     member type, it's a real flexible array member.  */
+  if (trailing && flexible_array_type_p (TREE_TYPE (TREE_OPERAND (ref, 0))))
+    return special_array_member::trail_flex;
+
+  /* The referenced member.  */
+  tree member = TREE_OPERAND (ref, 1);
+  tree memtype = TREE_TYPE (member);
+
+  /* If the type of the referenced member is not ARRAY, not a special
+     array member.  */
+  if (TREE_CODE (memtype) != ARRAY_TYPE)
+    return sam;
+
+  tree memsize = DECL_SIZE_UNIT (member);
+  if (memsize)
+    {
+      bool zero_length = integer_zerop (memsize);
+      if (!trailing && !zero_length)
+	/* MEMBER is either an interior array or is an array with
+	   more than one element, not a special array member.  */
+	return sam;
+
+      if (zero_length)
+	{
+	  if (trailing)
+	    sam = special_array_member::trail_0;
+	  else
+	    sam = special_array_member::int_0;
+	}
+      else
+	if (tree dom = TYPE_DOMAIN (memtype))
+	  if (tree min = TYPE_MIN_VALUE (dom))
+	    if (tree max = TYPE_MAX_VALUE (dom))
+	      if (TREE_CODE (min) == INTEGER_CST
+		  && TREE_CODE (max) == INTEGER_CST)
+		{
+		  offset_int minidx = wi::to_offset (min);
+		  offset_int maxidx = wi::to_offset (max);
+		  offset_int neltsm1 = maxidx - minidx;
+		  if (neltsm1 > 0)
+		    /* MEMBER is an array with a constant size which is more
+		       than one element, not a special array member.  */
+		    return sam;
+
+		  if (neltsm1 == 0 && trailing)
+		    sam = special_array_member::trail_1;
+		}
+      /* When getting here and the type has not been decided yet, the array is
+	 a variable length array.  */
+      if (sam == special_array_member::none)
+	  sam = special_array_member::is_vla;
+
+    }
+  return sam;
+}
+
+/* Check whether the component reference REF is a flexible array:
+   it's decided based on the value of flag_strict_flex_array.  */
+bool
+is_flexible_array_p (tree ref)
+{
+  gcc_assert (TREE_CODE (ref) == COMPONENT_REF);
+
+  bool is_trailing_array = array_at_struct_end_p (ref);
+  bool is_flexible_array = false;
+
+  /* Set for accesses to special trailing arrays.  */
+  special_array_member sam = special_array_member_type (ref);
+
+  switch (flag_strict_flex_array)
+    {
+      case 0:
+	/* default, all trailing arrays are flexiable arrays.  */
+	is_flexible_array = is_trailing_array;
+	break;
+      case 1:
+	/* Level 1: all [1], [0], and [] are flexiable arrays.  */
+	if (sam == special_array_member::trail_1)
+	  is_flexible_array = is_trailing_array;
+	/* FALLTHROUGH */
+      case 2:
+	/* Level 2: all [0], and [] are flexiable arrays.  */
+	if (sam == special_array_member::trail_0)
+	  is_flexible_array = is_trailing_array;
+	/* FALLTHROUGH */
+      case 3:
+	/* Level 3: Only [] are flexiable arrays.  */
+	if (sam == special_array_member::trail_flex)
+	  is_flexible_array = is_trailing_array;
+	break;
+      default:
+	gcc_unreachable ();
+    }
+
+  return is_flexible_array;
+}
+
 /* Return a tree representing the offset, in bytes, of the field referenced
    by EXP.  This does not include any offset in DECL_FIELD_BIT_OFFSET.  */
 
@@ -12831,12 +12937,9 @@ get_initializer_for (tree init, tree decl)
 tree
 component_ref_size (tree ref, special_array_member *sam /* = NULL */)
 {
-  gcc_assert (TREE_CODE (ref) == COMPONENT_REF);
-
-  special_array_member sambuf;
-  if (!sam)
-    sam = &sambuf;
-  *sam = special_array_member::none;
+  special_array_member temp_sam = special_array_member_type (ref);
+  if (sam)
+    *sam = temp_sam;
 
   /* The object/argument referenced by the COMPONENT_REF and its type.  */
   tree arg = TREE_OPERAND (ref, 0);
@@ -12857,41 +12960,13 @@ component_ref_size (tree ref, special_array_member *sam /* = NULL */)
 	return (tree_int_cst_equal (memsize, TYPE_SIZE_UNIT (memtype))
 		? memsize : NULL_TREE);
 
-      bool trailing = array_at_struct_end_p (ref);
-      bool zero_length = integer_zerop (memsize);
-      if (!trailing && !zero_length)
+      if (temp_sam == special_array_member::none)
 	/* MEMBER is either an interior array or is an array with
-	   more than one element.  */
+	   more than one element, i.e, a normal array.  */
 	return memsize;
 
-      if (zero_length)
-	{
-	  if (trailing)
-	    *sam = special_array_member::trail_0;
-	  else
-	    {
-	      *sam = special_array_member::int_0;
-	      memsize = NULL_TREE;
-	    }
-	}
-
-      if (!zero_length)
-	if (tree dom = TYPE_DOMAIN (memtype))
-	  if (tree min = TYPE_MIN_VALUE (dom))
-	    if (tree max = TYPE_MAX_VALUE (dom))
-	      if (TREE_CODE (min) == INTEGER_CST
-		  && TREE_CODE (max) == INTEGER_CST)
-		{
-		  offset_int minidx = wi::to_offset (min);
-		  offset_int maxidx = wi::to_offset (max);
-		  offset_int neltsm1 = maxidx - minidx;
-		  if (neltsm1 > 0)
-		    /* MEMBER is an array with more than one element.  */
-		    return memsize;
-
-		  if (neltsm1 == 0)
-		    *sam = special_array_member::trail_1;
-		}
+      if (temp_sam == special_array_member::int_0)
+	memsize = NULL_TREE;
 
       /* For a reference to a zero- or one-element array member of a union
 	 use the size of the union instead of the size of the member.  */
@@ -12908,7 +12983,7 @@ component_ref_size (tree ref, special_array_member *sam /* = NULL */)
   tree base = get_addr_base_and_unit_offset (ref, &baseoff);
   if (!base || !VAR_P (base))
     {
-      if (*sam != special_array_member::int_0)
+      if (temp_sam != special_array_member::int_0)
 	return NULL_TREE;
 
       if (TREE_CODE (arg) != COMPONENT_REF)
@@ -12928,7 +13003,7 @@ component_ref_size (tree ref, special_array_member *sam /* = NULL */)
   /* Determine the base type of the referenced object.  If it's
      the same as ARGTYPE and MEMBER has a known size, return it.  */
   tree bt = basetype;
-  if (*sam != special_array_member::int_0)
+  if (temp_sam != special_array_member::int_0)
     while (TREE_CODE (bt) == ARRAY_TYPE)
       bt = TREE_TYPE (bt);
   bool typematch = useless_type_conversion_p (argtype, bt);
@@ -12968,7 +13043,7 @@ component_ref_size (tree ref, special_array_member *sam /* = NULL */)
 	  if (DECL_P (base)
 	      && DECL_EXTERNAL (base)
 	      && bt == basetype
-	      && *sam != special_array_member::int_0)
+	      && temp_sam != special_array_member::int_0)
 	    /* The size of a flexible array member of an extern struct
 	       with no initializer cannot be determined (it's defined
 	       in another translation unit and can have an initializer
@@ -13017,7 +13092,7 @@ component_ref_size (tree ref, special_array_member *sam /* = NULL */)
 	  ? NULL_TREE : size_zero_node);
 }
 
-/* Return the machine mode of T.  For vectors, returns the mode of the
+/* Return the macmhine mode of T.  For vectors, returns the mode of the
    inner type.  The main use case is to feed the result to HONOR_NANS,
    avoiding the BLKmode that a direct TYPE_MODE (T) might return.  */
 
diff --git a/gcc/tree.h b/gcc/tree.h
index 507ea252b95..cd1c30ee179 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -5539,12 +5539,20 @@ extern tree component_ref_field_offset (tree);
    returns null.  */
 enum struct special_array_member
   {
-   none,      /* Not a special array member.  */
-   int_0,     /* Interior array member with size zero.  */
-   trail_0,   /* Trailing array member with size zero.  */
-   trail_1    /* Trailing array member with one element.  */
+   none,       /* Not a special array member.  */
+   is_vla,     /* variable length array.  */
+   int_0,      /* Interior array member with size zero.  */
+   trail_0,    /* Trailing array member with size zero.  */
+   trail_1,    /* Trailing array member with one element.  */
+   trail_flex  /* Flexible array member per C99 standard, [].  */
   };
 
+/* Return the type of special_array_member for the COMPONENT_REF REF.  */
+extern special_array_member special_array_member_type (tree);
+
+/* Check whether the component reference REF is a flexible array.  */
+extern bool is_flexible_array_p (tree);
+
 /* Return the size of the member referenced by the COMPONENT_REF, using
    its initializer expression if necessary in order to determine the size
    of an initialized flexible array member.  The size might be zero for
-- 
2.27.0

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Richard Biener]

On Mon, Jun 27, 2022 at 4:20 PM Qing Zhao via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi,
>
> Per our discussion in the bug report, I came up with the following patch:
>
> =======
>
> PR101836: Add a new option -fstrict-flex-array[=n]
>
> Add the new option and use it in __builtin_object_size.
>
> Treat the trailing array of a structure as a flexible array member in a
> stricter way.  The value of 'n' controls the level of strictness.
> 'n'=0 is the least strict, all trailing arrays of structures are treated
> as flexible array members; This is the default behavior of GCC without specify
> this option.
> 'n'=3 is the strictest, only when the trailing array is declared as a
> flexible array member per C99 standard onwards ([]), it is treated as a
> flexible array member;
> There are two more levels in between 0 and 3, which are provided to support
> older codes that use GCC zero-length array extension ([0]), or one-size array as
> flexible array member ([1]):
> When 'n' is 1, the trailing array is treated as a flexible array member
> when it is declared as either [], [0], or [1];
> When 'n' is 2, the trailing array is treated as a flexible array member
> when it is declared as either [], or [0].
>
> There are other places in GCC that conservatively treat flexible array members.
> A follow-up patch will make -ftrict-flex-array option to control all these
> places consistently.
>
> Bootstrapped and regression tested on both X86 and aarch64, no issues.
>
> Any comment and suggestion?

Since this aims at the C or C++ frontends but the middle-end eventually consumes
this it would be much nicer to encode this in the types themselves.
Since the least
strict reading is the default right now it would be a flag (on the
FIELD_DECL I suppose)
like DECL_NOT_FLEXARRAY or DECL_FIXED_SIZE?  Alternatively the flag could
also be on the record type enclosing the trailing array member (but
type sharing might
make this more difficult in the end).

There's also array_at_struct_end_p which is supposed to be the main
query interface
for this (but it seems people sneaked in more variants with eventually
different semantics ... :/)

Richard.



> Okay for commit to Gcc13?
>
> thanks.
>
> Qing
>
> =======================
>
> gcc/
>
>        PR tree-optimization/101836
>        * common.opt (fstrict-flex-array, fstrict-flex-array=): New options.
>        * doc/invoke.texi (-fstrict-flex-array, -fstrict-flex-array=): Document.
>        * tree-object-size.cc (addr_object_size): Call is_flexible_array_p to
>        check whether an array is a flexible array.
>        * tree.cc (special_array_member_type): New routine.
>        (is_flexible_array_p): New routine.
>        (component_ref_size): Call special_array_member_type to decide the
>        type of special array member.
>        * tree.h (enum struct special_array_member): Add is_vla, trail_flex.
>        (special_array_member_type): New prototype.
>        (is_flexible_array_p): New prototype.
>
> gcc/testsuite/
>
>        PR tree-optimization/101836
>        * gcc.dg/pr101836.c: New test.
>        * gcc.dg/pr101836_1.c: New test.
>        * gcc.dg/pr101836_2.c: New test.
>        * gcc.dg/pr101836_3.c: New test.
>        * gcc.dg/pr101836_4.c: New test.
>        * gcc.dg/pr101836_5.c: New test.
>
>
> The complete patch is:
>

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

Hi, Richard,


> On Jun 28, 2022, at 3:16 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Mon, Jun 27, 2022 at 4:20 PM Qing Zhao via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> 
>> Hi,
>> 
>> Per our discussion in the bug report, I came up with the following patch:
>> 
>> =======
>> 
>> PR101836: Add a new option -fstrict-flex-array[=n]
>> 
>> Add the new option and use it in __builtin_object_size.
>> 
>> Treat the trailing array of a structure as a flexible array member in a
>> stricter way.  The value of 'n' controls the level of strictness.
>> 'n'=0 is the least strict, all trailing arrays of structures are treated
>> as flexible array members; This is the default behavior of GCC without specify
>> this option.
>> 'n'=3 is the strictest, only when the trailing array is declared as a
>> flexible array member per C99 standard onwards ([]), it is treated as a
>> flexible array member;
>> There are two more levels in between 0 and 3, which are provided to support
>> older codes that use GCC zero-length array extension ([0]), or one-size array as
>> flexible array member ([1]):
>> When 'n' is 1, the trailing array is treated as a flexible array member
>> when it is declared as either [], [0], or [1];
>> When 'n' is 2, the trailing array is treated as a flexible array member
>> when it is declared as either [], or [0].
>> 
>> There are other places in GCC that conservatively treat flexible array members.
>> A follow-up patch will make -ftrict-flex-array option to control all these
>> places consistently.
>> 
>> Bootstrapped and regression tested on both X86 and aarch64, no issues.
>> 
>> Any comment and suggestion?
> 
> Since this aims at the C or C++ frontends but the middle-end eventually consumes
> this it would be much nicer to encode this in the types themselves.

Yes, I agree. 

Let the C/C++ FE to decide whether the [0], [1], or [] trailing array field of 
a structure is a flex array member or not based on the option -fstrict-flex-array and
then encode such info in the FIELD_DECL as flag DECL_NOT_FLEXARRAY. 

Later, the middle end just check the flag DECL_NOT_FLEXARRAY of the FIELD_DECL
to decide whether the trailing array is flexible array or not. 

This will eliminate all the hacks in the middle-end (as you mentioned, array_at_struct_end_p, 
and “trailing_array”, etc, and there are quite some phases use this routine to query, and in
an in-consistent way)

> Since the least
> strict reading is the default right now it would be a flag (on the
> FIELD_DECL I suppose)
> like DECL_NOT_FLEXARRAY or DECL_FIXED_SIZE?  Alternatively the flag could
> also be on the record type enclosing the trailing array member (but
> type sharing might
> make this more difficult in the end).
> 
> There's also array_at_struct_end_p which is supposed to be the main
> query interface
> for this (but it seems people sneaked in more variants with eventually
> different semantics ... :/)

Yes, there are many places right now that query “array_at_struct_end_p”, I was planning a follow-up
patchs to replace all “array_at_struct_end_p” with is_flexible_array_p. I guess that this follow-up patch 
will take quite some time to finish. 

So, my next step:

1. Update this current patch per your suggestion above, i.e, 

    A. Add a new flag (DECL_NOT_FLEXARRAY)  in FIELD_DECL, default is FALSE;
    B. In C/C++ FE,  for a trailing array field of a structure,  decide DECL_NOT_FLEXARRAY flag as following:
        Level 1:  any trailing array that is NOT [0], [1], [], DECL_NOT_FLEXARRAY is TRUE;
        Level 2:  any trailing array that is NOT [0], [], DECL_NOT_FLEXARRAY is TRUE; 
        Level 3:  any trailing array that is not [], DECL_NOT_FLEXARRAY is TRUE
    C. Use DECL_NOT_FLEXARRAY in tree-object-size.c  for __builtin_object_size to resolve bug PR101836.

2. Then replace all “array_at_struct_end_p” with using DECL_NOT_FLEXARRAY in GCC, adding new testing cases
For different phases with different level, resolving all regressions. 


I plan separate patches for 1 and 2.  Commit 1 first to enable kernel work as soon as possible. Then continue working
on 2 to make GCC consistent in gcc13. 

Let me know if you have any suggestion or comment.

Thanks

Qing
	 


> 
> Richard.
> 
> 
> 
>> Okay for commit to Gcc13?
>> 
>> thanks.
>> 
>> Qing
>> 
>> =======================
>> 
>> gcc/
>> 
>>       PR tree-optimization/101836
>>       * common.opt (fstrict-flex-array, fstrict-flex-array=): New options.
>>       * doc/invoke.texi (-fstrict-flex-array, -fstrict-flex-array=): Document.
>>       * tree-object-size.cc (addr_object_size): Call is_flexible_array_p to
>>       check whether an array is a flexible array.
>>       * tree.cc (special_array_member_type): New routine.
>>       (is_flexible_array_p): New routine.
>>       (component_ref_size): Call special_array_member_type to decide the
>>       type of special array member.
>>       * tree.h (enum struct special_array_member): Add is_vla, trail_flex.
>>       (special_array_member_type): New prototype.
>>       (is_flexible_array_p): New prototype.
>> 
>> gcc/testsuite/
>> 
>>       PR tree-optimization/101836
>>       * gcc.dg/pr101836.c: New test.
>>       * gcc.dg/pr101836_1.c: New test.
>>       * gcc.dg/pr101836_2.c: New test.
>>       * gcc.dg/pr101836_3.c: New test.
>>       * gcc.dg/pr101836_4.c: New test.
>>       * gcc.dg/pr101836_5.c: New test.
>> 
>> 
>> The complete patch is:

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Jakub Jelinek]

On Tue, Jun 28, 2022 at 03:03:12PM +0000, Qing Zhao wrote:
> 2. Then replace all “array_at_struct_end_p” with using DECL_NOT_FLEXARRAY in GCC, adding new testing cases

No, IMHO array_at_struct_end_p should stay as is, just test this extra flag
too.

	Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jun 28, 2022, at 11:08 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Tue, Jun 28, 2022 at 03:03:12PM +0000, Qing Zhao wrote:
>> 2. Then replace all “array_at_struct_end_p” with using DECL_NOT_FLEXARRAY in GCC, adding new testing cases
> 
> No, IMHO array_at_struct_end_p should stay as is, just test this extra flag
> too.

Could you please explain why we still need “array_at_struct_end_p” after we have the DECL_NOT_FLEXARRAY flag in FIELD_DECL?
In addition to serve the purpose to check whether an array might be a flexible array, what else this function “array_at_struct_end_p” will do?

Thanks.

Qing
> 
> 	Jakub
> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Martin Sebor]

On 6/28/22 01:16, Richard Biener wrote:
> On Mon, Jun 27, 2022 at 4:20 PM Qing Zhao via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Hi,
>>
>> Per our discussion in the bug report, I came up with the following patch:
>>
>> =======
>>
>> PR101836: Add a new option -fstrict-flex-array[=n]
>>
>> Add the new option and use it in __builtin_object_size.
>>
>> Treat the trailing array of a structure as a flexible array member in a
>> stricter way.  The value of 'n' controls the level of strictness.
>> 'n'=0 is the least strict, all trailing arrays of structures are treated
>> as flexible array members; This is the default behavior of GCC without specify
>> this option.
>> 'n'=3 is the strictest, only when the trailing array is declared as a
>> flexible array member per C99 standard onwards ([]), it is treated as a
>> flexible array member;
>> There are two more levels in between 0 and 3, which are provided to support
>> older codes that use GCC zero-length array extension ([0]), or one-size array as
>> flexible array member ([1]):
>> When 'n' is 1, the trailing array is treated as a flexible array member
>> when it is declared as either [], [0], or [1];
>> When 'n' is 2, the trailing array is treated as a flexible array member
>> when it is declared as either [], or [0].
>>
>> There are other places in GCC that conservatively treat flexible array members.
>> A follow-up patch will make -ftrict-flex-array option to control all these
>> places consistently.
>>
>> Bootstrapped and regression tested on both X86 and aarch64, no issues.
>>
>> Any comment and suggestion?
> 
> Since this aims at the C or C++ frontends but the middle-end eventually consumes
> this it would be much nicer to encode this in the types themselves.
> Since the least
> strict reading is the default right now it would be a flag (on the
> FIELD_DECL I suppose)
> like DECL_NOT_FLEXARRAY or DECL_FIXED_SIZE?  Alternatively the flag could
> also be on the record type enclosing the trailing array member (but
> type sharing might
> make this more difficult in the end).
> 
> There's also array_at_struct_end_p which is supposed to be the main
> query interface
> for this (but it seems people sneaked in more variants with eventually
> different semantics ... :/)

The conservative array_at_struct_end_p has historically been used
for codegen.  component_ref_size was added as a separate function
with more flexible (including stricter) semantics to implement
warnings without running the risk of interfering with codegen.

Martin

> 
> Richard.
> 
> 
> 
>> Okay for commit to Gcc13?
>>
>> thanks.
>>
>> Qing
>>
>> =======================
>>
>> gcc/
>>
>>         PR tree-optimization/101836
>>         * common.opt (fstrict-flex-array, fstrict-flex-array=): New options.
>>         * doc/invoke.texi (-fstrict-flex-array, -fstrict-flex-array=): Document.
>>         * tree-object-size.cc (addr_object_size): Call is_flexible_array_p to
>>         check whether an array is a flexible array.
>>         * tree.cc (special_array_member_type): New routine.
>>         (is_flexible_array_p): New routine.
>>         (component_ref_size): Call special_array_member_type to decide the
>>         type of special array member.
>>         * tree.h (enum struct special_array_member): Add is_vla, trail_flex.
>>         (special_array_member_type): New prototype.
>>         (is_flexible_array_p): New prototype.
>>
>> gcc/testsuite/
>>
>>         PR tree-optimization/101836
>>         * gcc.dg/pr101836.c: New test.
>>         * gcc.dg/pr101836_1.c: New test.
>>         * gcc.dg/pr101836_2.c: New test.
>>         * gcc.dg/pr101836_3.c: New test.
>>         * gcc.dg/pr101836_4.c: New test.
>>         * gcc.dg/pr101836_5.c: New test.
>>
>>
>> The complete patch is:
>>

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Jakub Jelinek]

On Tue, Jun 28, 2022 at 03:59:22PM +0000, Qing Zhao via Gcc-patches wrote:
> > On Jun 28, 2022, at 11:08 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > 
> > On Tue, Jun 28, 2022 at 03:03:12PM +0000, Qing Zhao wrote:
> >> 2. Then replace all “array_at_struct_end_p” with using DECL_NOT_FLEXARRAY in GCC, adding new testing cases
> > 
> > No, IMHO array_at_struct_end_p should stay as is, just test this extra flag
> > too.
> 
> Could you please explain why we still need “array_at_struct_end_p” after we have the DECL_NOT_FLEXARRAY flag in FIELD_DECL?

Because the flag just tells whether some array shouldn't be treated as (poor man's)
flexible array member.  We still need to find out if some FIELD_DECL is to
be treated like a flexible array member, which is a minority of
COMPONENT_REFs.
struct S { int a; char b[0]; int c; } s;
struct T { int d; char e[]; };
struct U { int f; struct T g; int h; } u;
Neither s.b nor u.g.e is to be treated like flexible array member,
no matter what -fstrict-flex-array= option is used.

	Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jun 28, 2022, at 12:43 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Tue, Jun 28, 2022 at 03:59:22PM +0000, Qing Zhao via Gcc-patches wrote:
>>> On Jun 28, 2022, at 11:08 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> 
>>> On Tue, Jun 28, 2022 at 03:03:12PM +0000, Qing Zhao wrote:
>>>> 2. Then replace all “array_at_struct_end_p” with using DECL_NOT_FLEXARRAY in GCC, adding new testing cases
>>> 
>>> No, IMHO array_at_struct_end_p should stay as is, just test this extra flag
>>> too.
>> 
>> Could you please explain why we still need “array_at_struct_end_p” after we have the DECL_NOT_FLEXARRAY flag in FIELD_DECL?
> 
> Because the flag just tells whether some array shouldn't be treated as (poor man's)
> flexible array member.  We still need to find out if some FIELD_DECL is to
> be treated like a flexible array member, which is a minority of
> COMPONENT_REFs.
> struct S { int a; char b[0]; int c; } s;
> struct T { int d; char e[]; };
> struct U { int f; struct T g; int h; } u;
> Neither s.b nor u.g.e is to be treated like flexible array member,
> no matter what -fstrict-flex-array= option is used.

Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?

The default is FALSE for all FIELD_DECL.

Only when the FIELD_DECL is a flexible array member, FE will set it to TRUE. 

i.e, FE needs to check

	1. The FIELD_DECL is the last field of the enclosing structure;
+      2. This FIELD_DECL is a flexible array per -fstrict-flex-array=n;

Then whenever the DECL_IS_FLEXARRAY is set to TRUE by FEs, that means the FIELD_DECL is an flexible array member.  
Middle-end then just use this flag to decide whether an array reference is a flexible array  or not.

Any comments?

Qing

> 
> 	Jakub
> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Jakub Jelinek]

On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
> > Because the flag just tells whether some array shouldn't be treated as (poor man's)
> > flexible array member.  We still need to find out if some FIELD_DECL is to
> > be treated like a flexible array member, which is a minority of
> > COMPONENT_REFs.
> > struct S { int a; char b[0]; int c; } s;
> > struct T { int d; char e[]; };
> > struct U { int f; struct T g; int h; } u;
> > Neither s.b nor u.g.e is to be treated like flexible array member,
> > no matter what -fstrict-flex-array= option is used.
> 
> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
> 
> The default is FALSE for all FIELD_DECL.

Doesn't matter whether it is positive or negative, you still need to analyze
it.  See the above example.  If you have struct T t; and test t.e, then it
is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
same FIELD_DECL.  In the t.e case e is the very last field, in the latter
case u.g.e is the last field in struct T, but struct U has the h field after
it.

	Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jun 28, 2022, at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
>>> Because the flag just tells whether some array shouldn't be treated as (poor man's)
>>> flexible array member.  We still need to find out if some FIELD_DECL is to
>>> be treated like a flexible array member, which is a minority of
>>> COMPONENT_REFs.
>>> struct S { int a; char b[0]; int c; } s;
>>> struct T { int d; char e[]; };
>>> struct U { int f; struct T g; int h; } u;
>>> Neither s.b nor u.g.e is to be treated like flexible array member,
>>> no matter what -fstrict-flex-array= option is used.
>> 
>> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
>> 
>> The default is FALSE for all FIELD_DECL.
> 
> Doesn't matter whether it is positive or negative, you still need to analyze
> it.  See the above example.  If you have struct T t; and test t.e, then it
> is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
> same FIELD_DECL.  In the t.e case e is the very last field, in the latter
> case u.g.e is the last field in struct T, but struct U has the h field after

So, do you mean that the current FE analysis will not be able to decide whether a specific array field is at the end of the enclosing structure? 
Only the middle end can decide this ?

Qing
> it.
> 
> 	Jakub
> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Jakub Jelinek]

On Tue, Jun 28, 2022 at 06:29:01PM +0000, Qing Zhao wrote:
> 
> 
> > On Jun 28, 2022, at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > 
> > On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
> >>> Because the flag just tells whether some array shouldn't be treated as (poor man's)
> >>> flexible array member.  We still need to find out if some FIELD_DECL is to
> >>> be treated like a flexible array member, which is a minority of
> >>> COMPONENT_REFs.
> >>> struct S { int a; char b[0]; int c; } s;
> >>> struct T { int d; char e[]; };
> >>> struct U { int f; struct T g; int h; } u;
> >>> Neither s.b nor u.g.e is to be treated like flexible array member,
> >>> no matter what -fstrict-flex-array= option is used.
> >> 
> >> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
> >> 
> >> The default is FALSE for all FIELD_DECL.
> > 
> > Doesn't matter whether it is positive or negative, you still need to analyze
> > it.  See the above example.  If you have struct T t; and test t.e, then it
> > is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
> > same FIELD_DECL.  In the t.e case e is the very last field, in the latter
> > case u.g.e is the last field in struct T, but struct U has the h field after
> 
> So, do you mean that the current FE analysis will not be able to decide whether a specific array field is at the end of the enclosing structure? 
> Only the middle end can decide this ?

Well, anything that analyzes it, can be in the FE or middle-end, but there
is no place to store it for later.

	Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jun 28, 2022, at 2:49 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Tue, Jun 28, 2022 at 06:29:01PM +0000, Qing Zhao wrote:
>> 
>> 
>>> On Jun 28, 2022, at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> 
>>> On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
>>>>> Because the flag just tells whether some array shouldn't be treated as (poor man's)
>>>>> flexible array member.  We still need to find out if some FIELD_DECL is to
>>>>> be treated like a flexible array member, which is a minority of
>>>>> COMPONENT_REFs.
>>>>> struct S { int a; char b[0]; int c; } s;
>>>>> struct T { int d; char e[]; };
>>>>> struct U { int f; struct T g; int h; } u;
>>>>> Neither s.b nor u.g.e is to be treated like flexible array member,
>>>>> no matter what -fstrict-flex-array= option is used.
>>>> 
>>>> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
>>>> 
>>>> The default is FALSE for all FIELD_DECL.
>>> 
>>> Doesn't matter whether it is positive or negative, you still need to analyze
>>> it.  See the above example.  If you have struct T t; and test t.e, then it
>>> is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
>>> same FIELD_DECL.  In the t.e case e is the very last field, in the latter
>>> case u.g.e is the last field in struct T, but struct U has the h field after
>> 
>> So, do you mean that the current FE analysis will not be able to decide whether a specific array field is at the end of the enclosing structure? 
>> Only the middle end can decide this ?
> 
> Well, anything that analyzes it, can be in the FE or middle-end, but there
> is no place to store it for later.

Then I am a little confused: 

If the FE can decide wether an array field is at the end of the enclosing structure,  then combined with whether it’s a [0], [1] or [], and which level of -fstrict-flex-array, 

The FE should be able to decide whether this array field is a flexible array member or not, then set the flag DECL_IS_FLEXARRAY (or DECL_NOT_FLEXARRAY). 

The new flag is the place to store such info, right?
Do I miss anything here?

Qing
> 
> 	Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

Hi, Jakub and Joseph:


> On Jun 28, 2022, at 12:43 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Tue, Jun 28, 2022 at 03:59:22PM +0000, Qing Zhao via Gcc-patches wrote:
>>> On Jun 28, 2022, at 11:08 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> 
>>> On Tue, Jun 28, 2022 at 03:03:12PM +0000, Qing Zhao wrote:
>>>> 2. Then replace all “array_at_struct_end_p” with using DECL_NOT_FLEXARRAY in GCC, adding new testing cases
>>> 
>>> No, IMHO array_at_struct_end_p should stay as is, just test this extra flag
>>> too.
>> 
>> Could you please explain why we still need “array_at_struct_end_p” after we have the DECL_NOT_FLEXARRAY flag in FIELD_DECL?
> 
> Because the flag just tells whether some array shouldn't be treated as (poor man's)
> flexible array member.  We still need to find out if some FIELD_DECL is to
> be treated like a flexible array member, which is a minority of
> COMPONENT_REFs.
> struct S { int a; char b[0]; int c; } s;
> struct T { int d; char e[]; };
> struct U { int f; struct T g; int h; } u;
> Neither s.b nor u.g.e is to be treated like flexible array member,
> no matter what -fstrict-flex-array= option is used.

I studied the above a little bit today with the following small testing cases:
[opc@qinzhao-ol8u3-x86 trailing_array]$ cat t1.c
struct AX
{
  int n;
  short ax[];
  int m;
};

void warn_ax_local (struct AX *p)
{
  p->ax[2] = 0;   
}
[opc@qinzhao-ol8u3-x86 trailing_array]$ /home/opc/Install/latest/bin/gcc -O2 -Wall  t1.c -S
t4.c:4:9: error: flexible array member not at end of struct
    4 |   short ax[];
      |         ^~

Looks like that it’s an error when the flexible array member is not at end of a struct.

However, for

[opc@qinzhao-ol8u3-x86 trailing_array]$ cat t2.c
struct AX
{
  int n;
  short ax[];
};

struct UX
{
  struct AX b;
  int m;
};

void warn_ax_local (struct AX *p, struct UX *q)
{
  p->ax[2] = 0;   
  q->b.ax[2] = 0;
}

[opc@qinzhao-ol8u3-x86 trailing_array]$  /home/opc/Install/latest/bin/gcc -O2 -Wall  t2.c -S
[opc@qinzhao-ol8u3-x86 trailing_array]$ 


My impression is:

C frontend is able to check whether the field is at the end of the structure and report error when it detect a flexible array member is not at the end of struct.

My question is:

1. Is the usage of flexible array member in struct UX of t2.c correct?
2. Why it’s correct?
3. If not correct, is this a bug in FE?

Thanks.

Qing

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Martin Sebor]

On 6/28/22 13:01, Qing Zhao wrote:
> 
> 
>> On Jun 28, 2022, at 2:49 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Tue, Jun 28, 2022 at 06:29:01PM +0000, Qing Zhao wrote:
>>>
>>>
>>>> On Jun 28, 2022, at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>
>>>> On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
>>>>>> Because the flag just tells whether some array shouldn't be treated as (poor man's)
>>>>>> flexible array member.  We still need to find out if some FIELD_DECL is to
>>>>>> be treated like a flexible array member, which is a minority of
>>>>>> COMPONENT_REFs.
>>>>>> struct S { int a; char b[0]; int c; } s;
>>>>>> struct T { int d; char e[]; };
>>>>>> struct U { int f; struct T g; int h; } u;
>>>>>> Neither s.b nor u.g.e is to be treated like flexible array member,
>>>>>> no matter what -fstrict-flex-array= option is used.
>>>>>
>>>>> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
>>>>>
>>>>> The default is FALSE for all FIELD_DECL.
>>>>
>>>> Doesn't matter whether it is positive or negative, you still need to analyze
>>>> it.  See the above example.  If you have struct T t; and test t.e, then it
>>>> is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
>>>> same FIELD_DECL.  In the t.e case e is the very last field, in the latter
>>>> case u.g.e is the last field in struct T, but struct U has the h field after
>>>
>>> So, do you mean that the current FE analysis will not be able to decide whether a specific array field is at the end of the enclosing structure?
>>> Only the middle end can decide this ?
>>
>> Well, anything that analyzes it, can be in the FE or middle-end, but there
>> is no place to store it for later.
> 
> Then I am a little confused:
> 
> If the FE can decide wether an array field is at the end of the enclosing structure,  then combined with whether it’s a [0], [1] or [], and which level of -fstrict-flex-array,
> 
> The FE should be able to decide whether this array field is a flexible array member or not, then set the flag DECL_IS_FLEXARRAY (or DECL_NOT_FLEXARRAY).
> 
> The new flag is the place to store such info, right?
> Do I miss anything here?

I think the problem is that there is just one FIELD_DECL for member
M of a given type T but there can be more than one instance of that
member, one in each struct that has a subobject of T as its own
member.  Whether M is or isn't a (valid) flexible array member
varies between the two instances.

Martin

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jun 29, 2022, at 5:14 PM, Martin Sebor <msebor@gmail.com> wrote:
> 
> On 6/28/22 13:01, Qing Zhao wrote:
>>> On Jun 28, 2022, at 2:49 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> 
>>> On Tue, Jun 28, 2022 at 06:29:01PM +0000, Qing Zhao wrote:
>>>> 
>>>> 
>>>>> On Jun 28, 2022, at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> 
>>>>> On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
>>>>>>> Because the flag just tells whether some array shouldn't be treated as (poor man's)
>>>>>>> flexible array member.  We still need to find out if some FIELD_DECL is to
>>>>>>> be treated like a flexible array member, which is a minority of
>>>>>>> COMPONENT_REFs.
>>>>>>> struct S { int a; char b[0]; int c; } s;
>>>>>>> struct T { int d; char e[]; };
>>>>>>> struct U { int f; struct T g; int h; } u;
>>>>>>> Neither s.b nor u.g.e is to be treated like flexible array member,
>>>>>>> no matter what -fstrict-flex-array= option is used.
>>>>>> 
>>>>>> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
>>>>>> 
>>>>>> The default is FALSE for all FIELD_DECL.
>>>>> 
>>>>> Doesn't matter whether it is positive or negative, you still need to analyze
>>>>> it.  See the above example.  If you have struct T t; and test t.e, then it
>>>>> is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
>>>>> same FIELD_DECL.  In the t.e case e is the very last field, in the latter
>>>>> case u.g.e is the last field in struct T, but struct U has the h field after
>>>> 
>>>> So, do you mean that the current FE analysis will not be able to decide whether a specific array field is at the end of the enclosing structure?
>>>> Only the middle end can decide this ?
>>> 
>>> Well, anything that analyzes it, can be in the FE or middle-end, but there
>>> is no place to store it for later.
>> Then I am a little confused:
>> If the FE can decide wether an array field is at the end of the enclosing structure,  then combined with whether it’s a [0], [1] or [], and which level of -fstrict-flex-array,
>> The FE should be able to decide whether this array field is a flexible array member or not, then set the flag DECL_IS_FLEXARRAY (or DECL_NOT_FLEXARRAY).
>> The new flag is the place to store such info, right?
>> Do I miss anything here?
> 
> I think the problem is that there is just one FIELD_DECL for member
> M of a given type T but there can be more than one instance of that
> member, one in each struct that has a subobject of T as its own
> member.  Whether M is or isn't a (valid) flexible array member
> varies between the two instances.

Okay, I see. 
A FIELD_DECL might be shared by multiple structure or unions, and whether 
it’s a flexible array member varies between different enclosing structures or unions.
Therefore FIELD_DECL cannot carry the flexible array member information accurately. 

Then, how about encoding the flexible array member information into the enclosing structure or union? 


Another thing is:  All this complexity is caused by GNU extension which permits the flexible array 
member not at the end of the struct. (As I mentioned in a previous email, I listed here again)

For example the following two examples:

1. [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t1.c
struct AX
{
  int n;
  short ax[];
  int m;
};

void warn_ax_local (struct AX *p)
{
  p->ax[2] = 0;   
}

2. [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t2.c
struct AX
{
  int n;
  short ax[];
};

struct UX
{
  struct AX b;
  int m;
};

void warn_ax_local (struct AX *p, struct UX *q)
{
  p->ax[2] = 0;   
  q->b.ax[2] = 0;
}

[opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t1.c -S
t4.c:4:9: error: flexible array member not at end of struct
    4 |   short ax[];

[opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t2.c -S

It’s clear to see that in the above t1.c,  GCC  reports error when the flexible array member is Not at the end of the structure  (AX) that immediately enclosing the field.
However, for t2.c, when the flexible array member is Not at the end of the structure that does not immediately enclosing it (UX), then it’s accepted.   

I am very confused about t2.c, is the struct UX a correct declaration? 

Thanks.

Qing

> 
> Martin

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Richard Biener]

> Am 30.06.2022 um 16:08 schrieb Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> 
> 
>> On Jun 29, 2022, at 5:14 PM, Martin Sebor <msebor@gmail.com> wrote:
>> 
>> On 6/28/22 13:01, Qing Zhao wrote:
>>>> On Jun 28, 2022, at 2:49 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> 
>>>> On Tue, Jun 28, 2022 at 06:29:01PM +0000, Qing Zhao wrote:
>>>>> 
>>>>> 
>>>>>> On Jun 28, 2022, at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>> 
>>>>>>> On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
>>>>>>>>> Because the flag just tells whether some array shouldn't be treated as (poor man's)
>>>>>>>>> flexible array member.  We still need to find out if some FIELD_DECL is to
>>>>>>>>> be treated like a flexible array member, which is a minority of
>>>>>>>>> COMPONENT_REFs.
>>>>>>>>> struct S { int a; char b[0]; int c; } s;
>>>>>>>>> struct T { int d; char e[]; };
>>>>>>>>> struct U { int f; struct T g; int h; } u;
>>>>>>>>> Neither s.b nor u.g.e is to be treated like flexible array member,
>>>>>>>>> no matter what -fstrict-flex-array= option is used.
>>>>>>>> 
>>>>>>>> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
>>>>>>>> 
>>>>>>>> The default is FALSE for all FIELD_DECL.
>>>>>>> 
>>>>>>> Doesn't matter whether it is positive or negative, you still need to analyze
>>>>>>> it.  See the above example.  If you have struct T t; and test t.e, then it
>>>>>>> is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
>>>>>>> same FIELD_DECL.  In the t.e case e is the very last field, in the latter
>>>>>>> case u.g.e is the last field in struct T, but struct U has the h field after
>>>>>> 
>>>>>> So, do you mean that the current FE analysis will not be able to decide whether a specific array field is at the end of the enclosing structure?
>>>>>> Only the middle end can decide this ?
>>>>> 
>>>>> Well, anything that analyzes it, can be in the FE or middle-end, but there
>>>>> is no place to store it for later.
>>> Then I am a little confused:
>>> If the FE can decide wether an array field is at the end of the enclosing structure,  then combined with whether it’s a [0], [1] or [], and which level of -fstrict-flex-array,
>>> The FE should be able to decide whether this array field is a flexible array member or not, then set the flag DECL_IS_FLEXARRAY (or DECL_NOT_FLEXARRAY).
>>> The new flag is the place to store such info, right?
>>> Do I miss anything here?
>> 
>> I think the problem is that there is just one FIELD_DECL for member
>> M of a given type T but there can be more than one instance of that
>> member, one in each struct that has a subobject of T as its own
>> member.  Whether M is or isn't a (valid) flexible array member
>> varies between the two instances.
> 
> Okay, I see. 
> A FIELD_DECL might be shared by multiple structure or unions, and whether 
> it’s a flexible array member varies between different enclosing structures or unions.
> Therefore FIELD_DECL cannot carry the flexible array member information accurately. 

No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.


> Then, how about encoding the flexible array member information into the enclosing structure or union? 
> 
> 
> Another thing is:  All this complexity is caused by GNU extension which permits the flexible array 
> member not at the end of the struct. (As I mentioned in a previous email, I listed here again)
> 
> For example the following two examples:
> 
> 1. [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t1.c
> struct AX
> {
>  int n;
>  short ax[];
>  int m;
> };
> 
> void warn_ax_local (struct AX *p)
> {
>  p->ax[2] = 0;   
> }
> 
> 2. [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t2.c
> struct AX
> {
>  int n;
>  short ax[];
> };
> 
> struct UX
> {
>  struct AX b;
>  int m;
> };
> 
> void warn_ax_local (struct AX *p, struct UX *q)
> {
>  p->ax[2] = 0;   
>  q->b.ax[2] = 0;
> }
> 
> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t1.c -S
> t4.c:4:9: error: flexible array member not at end of struct
>    4 |   short ax[];
> 
> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t2.c -S
> 
> It’s clear to see that in the above t1.c,  GCC  reports error when the flexible array member is Not at the end of the structure  (AX) that immediately enclosing the field.
> However, for t2.c, when the flexible array member is Not at the end of the structure that does not immediately enclosing it (UX), then it’s accepted.   
> 
> I am very confused about t2.c, is the struct UX a correct declaration? 
> 
> Thanks.
> 
> Qing
> 
>> 
>> Martin
> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jun 30, 2022, at 10:24 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> 
> 
>> Am 30.06.2022 um 16:08 schrieb Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org>:
>> 
>> 
>> 
>>> On Jun 29, 2022, at 5:14 PM, Martin Sebor <msebor@gmail.com> wrote:
>>> 
>>> On 6/28/22 13:01, Qing Zhao wrote:
>>>>> On Jun 28, 2022, at 2:49 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> 
>>>>> On Tue, Jun 28, 2022 at 06:29:01PM +0000, Qing Zhao wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Jun 28, 2022, at 2:22 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>>> 
>>>>>>>> On Tue, Jun 28, 2022 at 06:15:58PM +0000, Qing Zhao wrote:
>>>>>>>>>> Because the flag just tells whether some array shouldn't be treated as (poor man's)
>>>>>>>>>> flexible array member.  We still need to find out if some FIELD_DECL is to
>>>>>>>>>> be treated like a flexible array member, which is a minority of
>>>>>>>>>> COMPONENT_REFs.
>>>>>>>>>> struct S { int a; char b[0]; int c; } s;
>>>>>>>>>> struct T { int d; char e[]; };
>>>>>>>>>> struct U { int f; struct T g; int h; } u;
>>>>>>>>>> Neither s.b nor u.g.e is to be treated like flexible array member,
>>>>>>>>>> no matter what -fstrict-flex-array= option is used.
>>>>>>>>> 
>>>>>>>>> Then, to resolve this issue, we might need a opposite  flag DECL_IS_FLEXARRAY in FIELD_DECL?
>>>>>>>>> 
>>>>>>>>> The default is FALSE for all FIELD_DECL.
>>>>>>>> 
>>>>>>>> Doesn't matter whether it is positive or negative, you still need to analyze
>>>>>>>> it.  See the above example.  If you have struct T t; and test t.e, then it
>>>>>>>> is flexarray.  But u.g.e is not, even when the COMPONENT_REF refers to the
>>>>>>>> same FIELD_DECL.  In the t.e case e is the very last field, in the latter
>>>>>>>> case u.g.e is the last field in struct T, but struct U has the h field after
>>>>>>> 
>>>>>>> So, do you mean that the current FE analysis will not be able to decide whether a specific array field is at the end of the enclosing structure?
>>>>>>> Only the middle end can decide this ?
>>>>>> 
>>>>>> Well, anything that analyzes it, can be in the FE or middle-end, but there
>>>>>> is no place to store it for later.
>>>> Then I am a little confused:
>>>> If the FE can decide wether an array field is at the end of the enclosing structure,  then combined with whether it’s a [0], [1] or [], and which level of -fstrict-flex-array,
>>>> The FE should be able to decide whether this array field is a flexible array member or not, then set the flag DECL_IS_FLEXARRAY (or DECL_NOT_FLEXARRAY).
>>>> The new flag is the place to store such info, right?
>>>> Do I miss anything here?
>>> 
>>> I think the problem is that there is just one FIELD_DECL for member
>>> M of a given type T but there can be more than one instance of that
>>> member, one in each struct that has a subobject of T as its own
>>> member.  Whether M is or isn't a (valid) flexible array member
>>> varies between the two instances.
>> 
>> Okay, I see. 
>> A FIELD_DECL might be shared by multiple structure or unions, and whether 
>> it’s a flexible array member varies between different enclosing structures or unions.
>> Therefore FIELD_DECL cannot carry the flexible array member information accurately. 
> 
> No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.

Sorry for my dump questions: 

1. What do you mean by “cv variants” of a structure?
2. For the following example:

struct AX { int n; short ax[];};
struct UX {struct AX b; int m;};

Are there two different FIELD_DECLs in the IR, one for AX.ax, the other one is for UX.b.ax?

Qing

> 
> 
>> Then, how about encoding the flexible array member information into the enclosing structure or union? 
>> 
>> 
>> Another thing is:  All this complexity is caused by GNU extension which permits the flexible array 
>> member not at the end of the struct. (As I mentioned in a previous email, I listed here again)
>> 
>> For example the following two examples:
>> 
>> 1. [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t1.c
>> struct AX
>> {
>> int n;
>> short ax[];
>> int m;
>> };
>> 
>> void warn_ax_local (struct AX *p)
>> {
>> p->ax[2] = 0;   
>> }
>> 
>> 2. [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t2.c
>> struct AX
>> {
>> int n;
>> short ax[];
>> };
>> 
>> struct UX
>> {
>> struct AX b;
>> int m;
>> };
>> 
>> void warn_ax_local (struct AX *p, struct UX *q)
>> {
>> p->ax[2] = 0;   
>> q->b.ax[2] = 0;
>> }
>> 
>> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t1.c -S
>> t4.c:4:9: error: flexible array member not at end of struct
>>   4 |   short ax[];
>> 
>> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t2.c -S
>> 
>> It’s clear to see that in the above t1.c,  GCC  reports error when the flexible array member is Not at the end of the structure  (AX) that immediately enclosing the field.
>> However, for t2.c, when the flexible array member is Not at the end of the structure that does not immediately enclosing it (UX), then it’s accepted.   
>> 
>> I am very confused about t2.c, is the struct UX a correct declaration? 
>> 
>> Thanks.
>> 
>> Qing
>> 
>>> 
>>> Martin
>> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Jakub Jelinek]

On Thu, Jun 30, 2022 at 03:31:00PM +0000, Qing Zhao wrote:
> > No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.
> 
> Sorry for my dump questions: 
> 
> 1. What do you mean by “cv variants” of a structure?

const/volatile qualified variants.  So

> 2. For the following example:
> 
> struct AX { int n; short ax[];};

struct AX, const struct AX, volatile const struct AX etc. types will share
the FIELD_DECLs.

> struct UX {struct AX b; int m;};
> 
> Are there two different FIELD_DECLs in the IR, one for AX.ax, the other one is for UX.b.ax?

No, there are just n and ax FIELD_DECLs with DECL_CONTEXT of struct AX and
b and m FIELD_DECLs with DECL_CONTEXT of struct UX.

But, what is important is that when some FIELD_DECL is last in some
structure and has array type, it doesn't mean it should have an
unconstrained length.
In the above case, when struct AX is is followed by some other member, it
acts as a strict short ax[0]; field (even when that is an exception), one
can tak address of &UX.b.ax[0], but can't dereference that, or &UX.b.ax[1].

I believe pedantically flexible array members in such cases don't
necessarily mean zero length array, could be longer, e.g. for the usual
x86_64 alignments
struct BX { long long n; short o; short ax[]; };
struct VX { struct BX b; int m; };
I think it acts as short ax[3]; because the padding at the end of struct BX
is so long that 3 short elements fit in there.
While if one uses
struct BX bx = { 1LL, 2, { 3, 4, 5, 6, 7, 8, 9, 10 } };
(a GNU extension), then it acts as short ax[11]; - the initializer is 8
elements and after short ax[8]; is padding for another 3 full elemenets.
And of course:
struct BX *p = malloc (offsetof (struct BX, ax) + n * sizeof (short));
means short ax[n].
Whether struct WX { struct BX b; };
struct WX *p = malloc (offsetof (struct WX, b.ax) + n * sizeof (short));
is pedantically acting as short ax[n]; is unclear to me, but we are
generally allowing that and people expect it.

Though, on the GCC side, I think we are only treating like flexible arrays
what is really at the end of structs, not followed by other members.

	Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jun 30, 2022, at 1:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Thu, Jun 30, 2022 at 03:31:00PM +0000, Qing Zhao wrote:
>>> No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.
>> 
>> Sorry for my dump questions: 
>> 
>> 1. What do you mean by “cv variants” of a structure?
> 
> const/volatile qualified variants.  So
Okay. I see. thanks.
> 
>> 2. For the following example:
>> 
>> struct AX { int n; short ax[];};
> 
> struct AX, const struct AX, volatile const struct AX etc. types will share
> the FIELD_DECLs.

Okay. 
> 
>> struct UX {struct AX b; int m;};
>> 
>> Are there two different FIELD_DECLs in the IR, one for AX.ax, the other one is for UX.b.ax?
> 
> No, there are just n and ax FIELD_DECLs with DECL_CONTEXT of struct AX and
> b and m FIELD_DECLs with DECL_CONTEXT of struct UX.

Ah, right. 


> 
> But, what is important is that when some FIELD_DECL is last in some
> structure and has array type, it doesn't mean it should have an
> unconstrained length.
> In the above case, when struct AX is is followed by some other member, it
> acts as a strict short ax[0]; field (even when that is an exception), one
> can tak address of &UX.b.ax[0], but can't dereference that, or &UX.b.ax[1].

So, is this a GNU extension. I see that CLANG gives a warning by default and GCC gives a warning when specify -pedantic:
[opc@qinzhao-ol8u3-x86 trailing_array]$ cat t3.c
struct AX
{
  int n;
  short ax[];
};

struct UX
{
  struct AX b;
  int m;
};

void warn_ax_local (struct AX *p, struct UX *q)
{
  p->ax[2] = 0;   
  q->b.ax[2] = 0;
}
[opc@qinzhao-ol8u3-x86 trailing_array]$ clang -O2 -Wall t3.c -S
t3.c:9:13: warning: field 'b' with variable sized type 'struct AX' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
  struct AX b;
            ^
[opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t3.c -pedantic -S
t3.c:9:13: warning: invalid use of structure with flexible array member [-Wpedantic]
    9 |   struct AX b;
      |             ^

But, Yes, I agree, even though this is only a GNU extension, We still need to handle it and accept it as legal code. 

Then, yes, I also agree that encoding the info of is_flexible_array into FIELD_DECL is not good. 

How about encoding the info of “has_flexible_array” into the enclosing RECORD_TYPE or UNION_TYPE node?

For example, in the above example,  the RECORD_TYPE for “struct AX” will be marked as “has_flexible_array”, but that for “struct UX” will not.

> 
> I believe pedantically flexible array members in such cases don't
> necessarily mean zero length array, could be longer, e.g. for the usual
> x86_64 alignments
> struct BX { long long n; short o; short ax[]; };
> struct VX { struct BX b; int m; };
> I think it acts as short ax[3]; because the padding at the end of struct BX
> is so long that 3 short elements fit in there.
> While if one uses
> struct BX bx = { 1LL, 2, { 3, 4, 5, 6, 7, 8, 9, 10 } };
> (a GNU extension), then it acts as short ax[11]; - the initializer is 8
> elements and after short ax[8]; is padding for another 3 full elemenets.
> And of course:
> struct BX *p = malloc (offsetof (struct BX, ax) + n * sizeof (short));
> means short ax[n].
> Whether struct WX { struct BX b; };
> struct WX *p = malloc (offsetof (struct WX, b.ax) + n * sizeof (short));
> is pedantically acting as short ax[n]; is unclear to me, but we are
> generally allowing that and people expect it.

Okay, I see now.
> 
> Though, on the GCC side, I think we are only treating like flexible arrays
> what is really at the end of structs, not followed by other members.

My understanding is, Permitting flexible array to be followed by other members is a GNU extension.  (Actually, it’s not allowed by standard?).

Thanks a lot for your patience and help.

Qing
> 
> 	Jakub
> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Richard Biener]

On Thu, Jun 30, 2022 at 9:30 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>
>
>
> > On Jun 30, 2022, at 1:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 03:31:00PM +0000, Qing Zhao wrote:
> >>> No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.
> >>
> >> Sorry for my dump questions:
> >>
> >> 1. What do you mean by “cv variants” of a structure?
> >
> > const/volatile qualified variants.  So
> Okay. I see. thanks.
> >
> >> 2. For the following example:
> >>
> >> struct AX { int n; short ax[];};
> >
> > struct AX, const struct AX, volatile const struct AX etc. types will share
> > the FIELD_DECLs.
>
> Okay.
> >
> >> struct UX {struct AX b; int m;};
> >>
> >> Are there two different FIELD_DECLs in the IR, one for AX.ax, the other one is for UX.b.ax?
> >
> > No, there are just n and ax FIELD_DECLs with DECL_CONTEXT of struct AX and
> > b and m FIELD_DECLs with DECL_CONTEXT of struct UX.
>
> Ah, right.
>
>
> >
> > But, what is important is that when some FIELD_DECL is last in some
> > structure and has array type, it doesn't mean it should have an
> > unconstrained length.
> > In the above case, when struct AX is is followed by some other member, it
> > acts as a strict short ax[0]; field (even when that is an exception), one
> > can tak address of &UX.b.ax[0], but can't dereference that, or &UX.b.ax[1].
>
> So, is this a GNU extension. I see that CLANG gives a warning by default and GCC gives a warning when specify -pedantic:
> [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t3.c
> struct AX
> {
>   int n;
>   short ax[];
> };
>
> struct UX
> {
>   struct AX b;
>   int m;
> };
>
> void warn_ax_local (struct AX *p, struct UX *q)
> {
>   p->ax[2] = 0;
>   q->b.ax[2] = 0;
> }
> [opc@qinzhao-ol8u3-x86 trailing_array]$ clang -O2 -Wall t3.c -S
> t3.c:9:13: warning: field 'b' with variable sized type 'struct AX' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>   struct AX b;
>             ^
> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t3.c -pedantic -S
> t3.c:9:13: warning: invalid use of structure with flexible array member [-Wpedantic]
>     9 |   struct AX b;
>       |             ^
>
> But, Yes, I agree, even though this is only a GNU extension, We still need to handle it and accept it as legal code.
>
> Then, yes, I also agree that encoding the info of is_flexible_array into FIELD_DECL is not good.

Which is why I suggested to encode 'not_flexible_array'.  This way the
FE can mark all a[1] this way in some mode
but leave a[] as possibly flexarray (depending on context).

> How about encoding the info of “has_flexible_array” into the enclosing RECORD_TYPE or UNION_TYPE node?

But that has the same issue.  Consider

struct A { int n; int a[1]; };

where a is considered possibly a flexarray vs.

struct B { struct A a; int b; };

where B.a would be not considered to have a flexarray (again note
'possibly' vs. 'actually does').

Also

struct A a;

has 'a' as _not_ having a flexarray (because it's size is statically
allocated) but

struct A *a;
struct B *b;

a->a[n];

as possibly accessing the flexarray portion of *a while

b->a.a[n]

is not accessing a flexarray because there's a member after a in b.

For your original proposal it's really the field declaration itself
which changes so annotating the FIELD_DECL
seems correct to me.

> For example, in the above example,  the RECORD_TYPE for “struct AX” will be marked as “has_flexible_array”, but that for “struct UX” will not.
>
> >
> > I believe pedantically flexible array members in such cases don't
> > necessarily mean zero length array, could be longer, e.g. for the usual
> > x86_64 alignments
> > struct BX { long long n; short o; short ax[]; };
> > struct VX { struct BX b; int m; };
> > I think it acts as short ax[3]; because the padding at the end of struct BX
> > is so long that 3 short elements fit in there.
> > While if one uses
> > struct BX bx = { 1LL, 2, { 3, 4, 5, 6, 7, 8, 9, 10 } };
> > (a GNU extension), then it acts as short ax[11]; - the initializer is 8
> > elements and after short ax[8]; is padding for another 3 full elemenets.
> > And of course:
> > struct BX *p = malloc (offsetof (struct BX, ax) + n * sizeof (short));
> > means short ax[n].
> > Whether struct WX { struct BX b; };
> > struct WX *p = malloc (offsetof (struct WX, b.ax) + n * sizeof (short));
> > is pedantically acting as short ax[n]; is unclear to me, but we are
> > generally allowing that and people expect it.
>
> Okay, I see now.
> >
> > Though, on the GCC side, I think we are only treating like flexible arrays
> > what is really at the end of structs, not followed by other members.
>
> My understanding is, Permitting flexible array to be followed by other members is a GNU extension.  (Actually, it’s not allowed by standard?).
>
> Thanks a lot for your patience and help.
>
> Qing
> >
> >       Jakub
> >
>

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jul 1, 2022, at 2:49 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Thu, Jun 30, 2022 at 9:30 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>> 
>> 
>> 
>>> On Jun 30, 2022, at 1:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>> 
>>> On Thu, Jun 30, 2022 at 03:31:00PM +0000, Qing Zhao wrote:
>>>>> No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.
>>>> 
>>>> Sorry for my dump questions:
>>>> 
>>>> 1. What do you mean by “cv variants” of a structure?
>>> 
>>> const/volatile qualified variants.  So
>> Okay. I see. thanks.
>>> 
>>>> 2. For the following example:
>>>> 
>>>> struct AX { int n; short ax[];};
>>> 
>>> struct AX, const struct AX, volatile const struct AX etc. types will share
>>> the FIELD_DECLs.
>> 
>> Okay.
>>> 
>>>> struct UX {struct AX b; int m;};
>>>> 
>>>> Are there two different FIELD_DECLs in the IR, one for AX.ax, the other one is for UX.b.ax?
>>> 
>>> No, there are just n and ax FIELD_DECLs with DECL_CONTEXT of struct AX and
>>> b and m FIELD_DECLs with DECL_CONTEXT of struct UX.
>> 
>> Ah, right.
>> 
>> 
>>> 
>>> But, what is important is that when some FIELD_DECL is last in some
>>> structure and has array type, it doesn't mean it should have an
>>> unconstrained length.
>>> In the above case, when struct AX is is followed by some other member, it
>>> acts as a strict short ax[0]; field (even when that is an exception), one
>>> can tak address of &UX.b.ax[0], but can't dereference that, or &UX.b.ax[1].
>> 
>> So, is this a GNU extension. I see that CLANG gives a warning by default and GCC gives a warning when specify -pedantic:
>> [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t3.c
>> struct AX
>> {
>>  int n;
>>  short ax[];
>> };
>> 
>> struct UX
>> {
>>  struct AX b;
>>  int m;
>> };
>> 
>> void warn_ax_local (struct AX *p, struct UX *q)
>> {
>>  p->ax[2] = 0;
>>  q->b.ax[2] = 0;
>> }
>> [opc@qinzhao-ol8u3-x86 trailing_array]$ clang -O2 -Wall t3.c -S
>> t3.c:9:13: warning: field 'b' with variable sized type 'struct AX' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>>  struct AX b;
>>            ^
>> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t3.c -pedantic -S
>> t3.c:9:13: warning: invalid use of structure with flexible array member [-Wpedantic]
>>    9 |   struct AX b;
>>      |             ^
>> 
>> But, Yes, I agree, even though this is only a GNU extension, We still need to handle it and accept it as legal code.
>> 
>> Then, yes, I also agree that encoding the info of is_flexible_array into FIELD_DECL is not good.
> 
> Which is why I suggested to encode 'not_flexible_array'.  This way the
> FE can mark all a[1] this way in some mode
> but leave a[] as possibly flexarray (depending on context).

Then, FE marking (not_flexible_array) can not do the complete job to mark 
whether a field array is flexible array member or not,  Middle end still need to 
check the “context” (i.e, whether the array ref is at the end of a structure?)

So, only FE marking + Middle-end “context checking” together will decide a REAL flex array? 

If so, comparing to the current implemenation to have all the checking in middle-end, what’s the 
major benefit of moving part of the checking into FE, and leaving the other part in middle-end?

> 
>> How about encoding the info of “has_flexible_array” into the enclosing RECORD_TYPE or UNION_TYPE node?
> 
> But that has the same issue.  Consider
> 
> struct A { int n; int a[1]; };
> 
> where a is considered possibly a flexarray vs.
> 
> struct B { struct A a; int b; };
> 
> where B.a would be not considered to have a flexarray (again note
> 'possibly' vs. 'actually does').
> 
> Also
> 
> struct A a;
> 
> has 'a' as _not_ having a flexarray (because it's size is statically
> allocated) but
> 
> struct A *a;
> struct B *b;
> 
> a->a[n];
> 
> as possibly accessing the flexarray portion of *a while
> 
> b->a.a[n]
> 
> is not accessing a flexarray because there's a member after a in b.
> 
> For your original proposal it's really the field declaration itself
> which changes so annotating the FIELD_DECL
> seems correct to me.

Then middle-end still need to check the context, and combined 
with the “not_flexible_array” flag that is encoded in FIELD_DECL
 to make the final decision?

Thanks.

Qing
> 
>> For example, in the above example,  the RECORD_TYPE for “struct AX” will be marked as “has_flexible_array”, but that for “struct UX” will not.
>> 
>>> 
>>> I believe pedantically flexible array members in such cases don't
>>> necessarily mean zero length array, could be longer, e.g. for the usual
>>> x86_64 alignments
>>> struct BX { long long n; short o; short ax[]; };
>>> struct VX { struct BX b; int m; };
>>> I think it acts as short ax[3]; because the padding at the end of struct BX
>>> is so long that 3 short elements fit in there.
>>> While if one uses
>>> struct BX bx = { 1LL, 2, { 3, 4, 5, 6, 7, 8, 9, 10 } };
>>> (a GNU extension), then it acts as short ax[11]; - the initializer is 8
>>> elements and after short ax[8]; is padding for another 3 full elemenets.
>>> And of course:
>>> struct BX *p = malloc (offsetof (struct BX, ax) + n * sizeof (short));
>>> means short ax[n].
>>> Whether struct WX { struct BX b; };
>>> struct WX *p = malloc (offsetof (struct WX, b.ax) + n * sizeof (short));
>>> is pedantically acting as short ax[n]; is unclear to me, but we are
>>> generally allowing that and people expect it.
>> 
>> Okay, I see now.
>>> 
>>> Though, on the GCC side, I think we are only treating like flexible arrays
>>> what is really at the end of structs, not followed by other members.
>> 
>> My understanding is, Permitting flexible array to be followed by other members is a GNU extension.  (Actually, it’s not allowed by standard?).
>> 
>> Thanks a lot for your patience and help.
>> 
>> Qing
>>> 
>>>      Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Richard Biener]

On Fri, Jul 1, 2022 at 2:55 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>
>
>
> > On Jul 1, 2022, at 2:49 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Thu, Jun 30, 2022 at 9:30 PM Qing Zhao <qing.zhao@oracle.com> wrote:
> >>
> >>
> >>
> >>> On Jun 30, 2022, at 1:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> >>>
> >>> On Thu, Jun 30, 2022 at 03:31:00PM +0000, Qing Zhao wrote:
> >>>>> No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.
> >>>>
> >>>> Sorry for my dump questions:
> >>>>
> >>>> 1. What do you mean by “cv variants” of a structure?
> >>>
> >>> const/volatile qualified variants.  So
> >> Okay. I see. thanks.
> >>>
> >>>> 2. For the following example:
> >>>>
> >>>> struct AX { int n; short ax[];};
> >>>
> >>> struct AX, const struct AX, volatile const struct AX etc. types will share
> >>> the FIELD_DECLs.
> >>
> >> Okay.
> >>>
> >>>> struct UX {struct AX b; int m;};
> >>>>
> >>>> Are there two different FIELD_DECLs in the IR, one for AX.ax, the other one is for UX.b.ax?
> >>>
> >>> No, there are just n and ax FIELD_DECLs with DECL_CONTEXT of struct AX and
> >>> b and m FIELD_DECLs with DECL_CONTEXT of struct UX.
> >>
> >> Ah, right.
> >>
> >>
> >>>
> >>> But, what is important is that when some FIELD_DECL is last in some
> >>> structure and has array type, it doesn't mean it should have an
> >>> unconstrained length.
> >>> In the above case, when struct AX is is followed by some other member, it
> >>> acts as a strict short ax[0]; field (even when that is an exception), one
> >>> can tak address of &UX.b.ax[0], but can't dereference that, or &UX.b.ax[1].
> >>
> >> So, is this a GNU extension. I see that CLANG gives a warning by default and GCC gives a warning when specify -pedantic:
> >> [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t3.c
> >> struct AX
> >> {
> >>  int n;
> >>  short ax[];
> >> };
> >>
> >> struct UX
> >> {
> >>  struct AX b;
> >>  int m;
> >> };
> >>
> >> void warn_ax_local (struct AX *p, struct UX *q)
> >> {
> >>  p->ax[2] = 0;
> >>  q->b.ax[2] = 0;
> >> }
> >> [opc@qinzhao-ol8u3-x86 trailing_array]$ clang -O2 -Wall t3.c -S
> >> t3.c:9:13: warning: field 'b' with variable sized type 'struct AX' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
> >>  struct AX b;
> >>            ^
> >> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t3.c -pedantic -S
> >> t3.c:9:13: warning: invalid use of structure with flexible array member [-Wpedantic]
> >>    9 |   struct AX b;
> >>      |             ^
> >>
> >> But, Yes, I agree, even though this is only a GNU extension, We still need to handle it and accept it as legal code.
> >>
> >> Then, yes, I also agree that encoding the info of is_flexible_array into FIELD_DECL is not good.
> >
> > Which is why I suggested to encode 'not_flexible_array'.  This way the
> > FE can mark all a[1] this way in some mode
> > but leave a[] as possibly flexarray (depending on context).
>
> Then, FE marking (not_flexible_array) can not do the complete job to mark
> whether a field array is flexible array member or not,  Middle end still need to
> check the “context” (i.e, whether the array ref is at the end of a structure?)

Yes, but at the very "root" the frontend get's to say whether char[1]
is possibly
flexarray or if only char[] is.

> So, only FE marking + Middle-end “context checking” together will decide a REAL flex array?
>
> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?

Because a frontend might decide based on language rules that char[1]
is never a flexarray and
in particular it can decide to do that only for user declared
structures.  In particular the latter is
difficult for the middle-end where some aggregates are built by the
middle-end (gcov) or the
targets.

> >
> >> How about encoding the info of “has_flexible_array” into the enclosing RECORD_TYPE or UNION_TYPE node?
> >
> > But that has the same issue.  Consider
> >
> > struct A { int n; int a[1]; };
> >
> > where a is considered possibly a flexarray vs.
> >
> > struct B { struct A a; int b; };
> >
> > where B.a would be not considered to have a flexarray (again note
> > 'possibly' vs. 'actually does').
> >
> > Also
> >
> > struct A a;
> >
> > has 'a' as _not_ having a flexarray (because it's size is statically
> > allocated) but
> >
> > struct A *a;
> > struct B *b;
> >
> > a->a[n];
> >
> > as possibly accessing the flexarray portion of *a while
> >
> > b->a.a[n]
> >
> > is not accessing a flexarray because there's a member after a in b.
> >
> > For your original proposal it's really the field declaration itself
> > which changes so annotating the FIELD_DECL
> > seems correct to me.
>
> Then middle-end still need to check the context, and combined
> with the “not_flexible_array” flag that is encoded in FIELD_DECL
>  to make the final decision?

Yes.

> Thanks.
>
> Qing
> >
> >> For example, in the above example,  the RECORD_TYPE for “struct AX” will be marked as “has_flexible_array”, but that for “struct UX” will not.
> >>
> >>>
> >>> I believe pedantically flexible array members in such cases don't
> >>> necessarily mean zero length array, could be longer, e.g. for the usual
> >>> x86_64 alignments
> >>> struct BX { long long n; short o; short ax[]; };
> >>> struct VX { struct BX b; int m; };
> >>> I think it acts as short ax[3]; because the padding at the end of struct BX
> >>> is so long that 3 short elements fit in there.
> >>> While if one uses
> >>> struct BX bx = { 1LL, 2, { 3, 4, 5, 6, 7, 8, 9, 10 } };
> >>> (a GNU extension), then it acts as short ax[11]; - the initializer is 8
> >>> elements and after short ax[8]; is padding for another 3 full elemenets.
> >>> And of course:
> >>> struct BX *p = malloc (offsetof (struct BX, ax) + n * sizeof (short));
> >>> means short ax[n].
> >>> Whether struct WX { struct BX b; };
> >>> struct WX *p = malloc (offsetof (struct WX, b.ax) + n * sizeof (short));
> >>> is pedantically acting as short ax[n]; is unclear to me, but we are
> >>> generally allowing that and people expect it.
> >>
> >> Okay, I see now.
> >>>
> >>> Though, on the GCC side, I think we are only treating like flexible arrays
> >>> what is really at the end of structs, not followed by other members.
> >>
> >> My understanding is, Permitting flexible array to be followed by other members is a GNU extension.  (Actually, it’s not allowed by standard?).
> >>
> >> Thanks a lot for your patience and help.
> >>
> >> Qing
> >>>
> >>>      Jakub
>

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Jakub Jelinek]

On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the 
> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?

The point is recording early what FIELD_DECLs could be vs. can't possibly be
treated like flexible array members and just use that flag in the decisions
in the current routines in addition to what it is doing.

	Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jul 1, 2022, at 8:58 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Fri, Jul 1, 2022 at 2:55 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>> 
>> 
>> 
>>> On Jul 1, 2022, at 2:49 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>> 
>>> On Thu, Jun 30, 2022 at 9:30 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Jun 30, 2022, at 1:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> 
>>>>> On Thu, Jun 30, 2022 at 03:31:00PM +0000, Qing Zhao wrote:
>>>>>>> No, that’s not true.  A FIELD_DELC is only shared for cv variants of a structure.
>>>>>> 
>>>>>> Sorry for my dump questions:
>>>>>> 
>>>>>> 1. What do you mean by “cv variants” of a structure?
>>>>> 
>>>>> const/volatile qualified variants.  So
>>>> Okay. I see. thanks.
>>>>> 
>>>>>> 2. For the following example:
>>>>>> 
>>>>>> struct AX { int n; short ax[];};
>>>>> 
>>>>> struct AX, const struct AX, volatile const struct AX etc. types will share
>>>>> the FIELD_DECLs.
>>>> 
>>>> Okay.
>>>>> 
>>>>>> struct UX {struct AX b; int m;};
>>>>>> 
>>>>>> Are there two different FIELD_DECLs in the IR, one for AX.ax, the other one is for UX.b.ax?
>>>>> 
>>>>> No, there are just n and ax FIELD_DECLs with DECL_CONTEXT of struct AX and
>>>>> b and m FIELD_DECLs with DECL_CONTEXT of struct UX.
>>>> 
>>>> Ah, right.
>>>> 
>>>> 
>>>>> 
>>>>> But, what is important is that when some FIELD_DECL is last in some
>>>>> structure and has array type, it doesn't mean it should have an
>>>>> unconstrained length.
>>>>> In the above case, when struct AX is is followed by some other member, it
>>>>> acts as a strict short ax[0]; field (even when that is an exception), one
>>>>> can tak address of &UX.b.ax[0], but can't dereference that, or &UX.b.ax[1].
>>>> 
>>>> So, is this a GNU extension. I see that CLANG gives a warning by default and GCC gives a warning when specify -pedantic:
>>>> [opc@qinzhao-ol8u3-x86 trailing_array]$ cat t3.c
>>>> struct AX
>>>> {
>>>> int n;
>>>> short ax[];
>>>> };
>>>> 
>>>> struct UX
>>>> {
>>>> struct AX b;
>>>> int m;
>>>> };
>>>> 
>>>> void warn_ax_local (struct AX *p, struct UX *q)
>>>> {
>>>> p->ax[2] = 0;
>>>> q->b.ax[2] = 0;
>>>> }
>>>> [opc@qinzhao-ol8u3-x86 trailing_array]$ clang -O2 -Wall t3.c -S
>>>> t3.c:9:13: warning: field 'b' with variable sized type 'struct AX' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
>>>> struct AX b;
>>>>           ^
>>>> [opc@qinzhao-ol8u3-x86 trailing_array]$ gcc -O2 -Wall t3.c -pedantic -S
>>>> t3.c:9:13: warning: invalid use of structure with flexible array member [-Wpedantic]
>>>>   9 |   struct AX b;
>>>>     |             ^
>>>> 
>>>> But, Yes, I agree, even though this is only a GNU extension, We still need to handle it and accept it as legal code.
>>>> 
>>>> Then, yes, I also agree that encoding the info of is_flexible_array into FIELD_DECL is not good.
>>> 
>>> Which is why I suggested to encode 'not_flexible_array'.  This way the
>>> FE can mark all a[1] this way in some mode
>>> but leave a[] as possibly flexarray (depending on context).
>> 
>> Then, FE marking (not_flexible_array) can not do the complete job to mark
>> whether a field array is flexible array member or not,  Middle end still need to
>> check the “context” (i.e, whether the array ref is at the end of a structure?)
> 
> Yes, but at the very "root" the frontend get's to say whether char[1]
> is possibly
> flexarray or if only char[] is.

Okay. 
> 
>> So, only FE marking + Middle-end “context checking” together will decide a REAL flex array?
>> 
>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
> 
> Because a frontend might decide based on language rules that char[1]
> is never a flexarray and
> in particular it can decide to do that only for user declared
> structures.  In particular the latter is
> difficult for the middle-end where some aggregates are built by the
> middle-end (gcov) or the
> targets.

That makes sense. 
> 
>>> 
>>>> How about encoding the info of “has_flexible_array” into the enclosing RECORD_TYPE or UNION_TYPE node?
>>> 
>>> But that has the same issue.  Consider
>>> 
>>> struct A { int n; int a[1]; };
>>> 
>>> where a is considered possibly a flexarray vs.
>>> 
>>> struct B { struct A a; int b; };
>>> 
>>> where B.a would be not considered to have a flexarray (again note
>>> 'possibly' vs. 'actually does').
>>> 
>>> Also
>>> 
>>> struct A a;
>>> 
>>> has 'a' as _not_ having a flexarray (because it's size is statically
>>> allocated) but
>>> 
>>> struct A *a;
>>> struct B *b;
>>> 
>>> a->a[n];
>>> 
>>> as possibly accessing the flexarray portion of *a while
>>> 
>>> b->a.a[n]
>>> 
>>> is not accessing a flexarray because there's a member after a in b.
>>> 
>>> For your original proposal it's really the field declaration itself
>>> which changes so annotating the FIELD_DECL
>>> seems correct to me.
>> 
>> Then middle-end still need to check the context, and combined
>> with the “not_flexible_array” flag that is encoded in FIELD_DECL
>> to make the final decision?
> 
> Yes.

Okay, I see now.

thanks.

Qing
> 
>> Thanks.
>> 
>> Qing
>>> 
>>>> For example, in the above example,  the RECORD_TYPE for “struct AX” will be marked as “has_flexible_array”, but that for “struct UX” will not.
>>>> 
>>>>> 
>>>>> I believe pedantically flexible array members in such cases don't
>>>>> necessarily mean zero length array, could be longer, e.g. for the usual
>>>>> x86_64 alignments
>>>>> struct BX { long long n; short o; short ax[]; };
>>>>> struct VX { struct BX b; int m; };
>>>>> I think it acts as short ax[3]; because the padding at the end of struct BX
>>>>> is so long that 3 short elements fit in there.
>>>>> While if one uses
>>>>> struct BX bx = { 1LL, 2, { 3, 4, 5, 6, 7, 8, 9, 10 } };
>>>>> (a GNU extension), then it acts as short ax[11]; - the initializer is 8
>>>>> elements and after short ax[8]; is padding for another 3 full elemenets.
>>>>> And of course:
>>>>> struct BX *p = malloc (offsetof (struct BX, ax) + n * sizeof (short));
>>>>> means short ax[n].
>>>>> Whether struct WX { struct BX b; };
>>>>> struct WX *p = malloc (offsetof (struct WX, b.ax) + n * sizeof (short));
>>>>> is pedantically acting as short ax[n]; is unclear to me, but we are
>>>>> generally allowing that and people expect it.
>>>> 
>>>> Okay, I see now.
>>>>> 
>>>>> Though, on the GCC side, I think we are only treating like flexible arrays
>>>>> what is really at the end of structs, not followed by other members.
>>>> 
>>>> My understanding is, Permitting flexible array to be followed by other members is a GNU extension.  (Actually, it’s not allowed by standard?).
>>>> 
>>>> Thanks a lot for your patience and help.
>>>> 
>>>> Qing
>>>>> 
>>>>>     Jakub

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jul 1, 2022, at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the 
>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
> 
> The point is recording early what FIELD_DECLs could be vs. can't possibly be
> treated like flexible array members and just use that flag in the decisions
> in the current routines in addition to what it is doing.

Okay. 

Based on the discussion so far, I will do the following:

1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
    [] and the option -fstrict-flex-array, and whether it’s the last field of the DECL_CONTEXT.
3. In Middle end,  Add a new utility routine is_flexible_array_member_p, which bases on 
    DECL_NOT_FLEXARRAY + array_at_struct_end_p to decide whether the array
    reference is a real flexible array member reference. 


Middle end currently is quite mess, array_at_struct_end_p, component_ref_size, and all the phases that
use these routines need to be updated, + new testing cases for each of the phases.


So, I still plan to separate the patch set into 2 parts:

  Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
                 Then kernel can use __FORTIFY_SOURCE correctly;

  Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.

Let me know if you have any comment and suggestion.

Thanks a lot for all your help.

Qing

> 
> 	Jakub
> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Martin Sebor]

On 7/1/22 08:01, Qing Zhao wrote:
> 
> 
>> On Jul 1, 2022, at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
>>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
>>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
>>
>> The point is recording early what FIELD_DECLs could be vs. can't possibly be
>> treated like flexible array members and just use that flag in the decisions
>> in the current routines in addition to what it is doing.
> 
> Okay.
> 
> Based on the discussion so far, I will do the following:
> 
> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
>      [] and the option -fstrict-flex-array, and whether it’s the last field of the DECL_CONTEXT.
> 3. In Middle end,  Add a new utility routine is_flexible_array_member_p, which bases on
>      DECL_NOT_FLEXARRAY + array_at_struct_end_p to decide whether the array
>      reference is a real flexible array member reference.
> 
> 
> Middle end currently is quite mess, array_at_struct_end_p, component_ref_size, and all the phases that
> use these routines need to be updated, + new testing cases for each of the phases.
> 
> 
> So, I still plan to separate the patch set into 2 parts:
> 
>    Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
>                   Then kernel can use __FORTIFY_SOURCE correctly;
> 
>    Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
> 
> Let me know if you have any comment and suggestion.

It might be worth considering whether it should be possible to control
the "flexible array" property separately for each trailing array member
via either a #pragma or an attribute in headers that can't change
the struct layout but that need to be usable in programs compiled with
stricter -fstrict-flex-array=N settings.

Martin

> 
> Thanks a lot for all your help.
> 
> Qing
> 
>>
>> 	Jakub
>>
> 

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Richard Biener]

On Fri, Jul 1, 2022 at 5:32 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 7/1/22 08:01, Qing Zhao wrote:
> >
> >
> >> On Jul 1, 2022, at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> >>
> >> On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
> >>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
> >>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
> >>
> >> The point is recording early what FIELD_DECLs could be vs. can't possibly be
> >> treated like flexible array members and just use that flag in the decisions
> >> in the current routines in addition to what it is doing.
> >
> > Okay.
> >
> > Based on the discussion so far, I will do the following:
> >
> > 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
> > 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
> >      [] and the option -fstrict-flex-array, and whether it’s the last field of the DECL_CONTEXT.
> > 3. In Middle end,  Add a new utility routine is_flexible_array_member_p, which bases on
> >      DECL_NOT_FLEXARRAY + array_at_struct_end_p to decide whether the array
> >      reference is a real flexible array member reference.

I would just update all existing users, not introduce another wrapper
that takes DECL_NOT_FLEXARRAY
into account additionally.

> >
> >
> > Middle end currently is quite mess, array_at_struct_end_p, component_ref_size, and all the phases that
> > use these routines need to be updated, + new testing cases for each of the phases.
> >
> >
> > So, I still plan to separate the patch set into 2 parts:
> >
> >    Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
> >                   Then kernel can use __FORTIFY_SOURCE correctly;
> >
> >    Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
> >
> > Let me know if you have any comment and suggestion.
>
> It might be worth considering whether it should be possible to control
> the "flexible array" property separately for each trailing array member
> via either a #pragma or an attribute in headers that can't change
> the struct layout but that need to be usable in programs compiled with
> stricter -fstrict-flex-array=N settings.

Or an decl attribute.

Richard.

>
> Martin
>
> >
> > Thanks a lot for all your help.
> >
> > Qing
> >
> >>
> >>      Jakub
> >>
> >
>

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

(Sorry for the late reply, just came back from a short vacation.)

> On Jul 4, 2022, at 2:49 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Fri, Jul 1, 2022 at 5:32 PM Martin Sebor <msebor@gmail.com> wrote:
>> 
>> On 7/1/22 08:01, Qing Zhao wrote:
>>> 
>>> 
>>>> On Jul 1, 2022, at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> 
>>>> On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
>>>>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
>>>>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
>>>> 
>>>> The point is recording early what FIELD_DECLs could be vs. can't possibly be
>>>> treated like flexible array members and just use that flag in the decisions
>>>> in the current routines in addition to what it is doing.
>>> 
>>> Okay.
>>> 
>>> Based on the discussion so far, I will do the following:
>>> 
>>> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
>>> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
>>>     [] and the option -fstrict-flex-array, and whether it’s the last field of the DECL_CONTEXT.
>>> 3. In Middle end,  Add a new utility routine is_flexible_array_member_p, which bases on
>>>     DECL_NOT_FLEXARRAY + array_at_struct_end_p to decide whether the array
>>>     reference is a real flexible array member reference.
> 
> I would just update all existing users, not introduce another wrapper
> that takes DECL_NOT_FLEXARRAY
> into account additionally.

Okay. 
> 
>>> 
>>> 
>>> Middle end currently is quite mess, array_at_struct_end_p, component_ref_size, and all the phases that
>>> use these routines need to be updated, + new testing cases for each of the phases.
>>> 
>>> 
>>> So, I still plan to separate the patch set into 2 parts:
>>> 
>>>   Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
>>>                  Then kernel can use __FORTIFY_SOURCE correctly;
>>> 
>>>   Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
>>> 
>>> Let me know if you have any comment and suggestion.
>> 
>> It might be worth considering whether it should be possible to control
>> the "flexible array" property separately for each trailing array member
>> via either a #pragma or an attribute in headers that can't change
>> the struct layout but that need to be usable in programs compiled with
>> stricter -fstrict-flex-array=N settings.
> 
> Or an decl attribute.

Yes, it might be necessary to add a corresponding decl attribute 

strict_flex_array (N)

Which is attached to a trailing structure array member to provide the user a finer control when -fstrict-flex-array=N is specified. 

So, I will do the following:


*****User interface:

1. command line option:
     -fstrict-flex-array=N       (N=0, 1, 2, 3)
2.  decl attribute:
     strict_flex_array (N)      (N=0, 1, 2, 3)


*****Implementation:

1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
     [], the option -fstrict-flex-array, the attribute strict_flex_array,  and whether it’s the last field 
     of the DECL_CONTEXT.
3. In Middle end,   update all users of “array_at_struct_end_p" or “component_ref_size”, or any place that treats
    Trailing array as flexible array member with the new flag  DECL_NOT_FLEXARRAY. 
    (Still think we need a new consistent utility routine here). 


I still plan to separate the patch set into 2 parts:

Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
	       Then kernel can use __FORTIFY_SOURCE correctly.
Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.


Let me know any more comment or suggestion.

Thanks a lot.

Qing

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Richard Biener]

On Wed, Jul 6, 2022 at 4:20 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>
> (Sorry for the late reply, just came back from a short vacation.)
>
> > On Jul 4, 2022, at 2:49 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> >
> > On Fri, Jul 1, 2022 at 5:32 PM Martin Sebor <msebor@gmail.com> wrote:
> >>
> >> On 7/1/22 08:01, Qing Zhao wrote:
> >>>
> >>>
> >>>> On Jul 1, 2022, at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> >>>>
> >>>> On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
> >>>>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
> >>>>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
> >>>>
> >>>> The point is recording early what FIELD_DECLs could be vs. can't possibly be
> >>>> treated like flexible array members and just use that flag in the decisions
> >>>> in the current routines in addition to what it is doing.
> >>>
> >>> Okay.
> >>>
> >>> Based on the discussion so far, I will do the following:
> >>>
> >>> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
> >>> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
> >>>     [] and the option -fstrict-flex-array, and whether it’s the last field of the DECL_CONTEXT.
> >>> 3. In Middle end,  Add a new utility routine is_flexible_array_member_p, which bases on
> >>>     DECL_NOT_FLEXARRAY + array_at_struct_end_p to decide whether the array
> >>>     reference is a real flexible array member reference.
> >
> > I would just update all existing users, not introduce another wrapper
> > that takes DECL_NOT_FLEXARRAY
> > into account additionally.
>
> Okay.
> >
> >>>
> >>>
> >>> Middle end currently is quite mess, array_at_struct_end_p, component_ref_size, and all the phases that
> >>> use these routines need to be updated, + new testing cases for each of the phases.
> >>>
> >>>
> >>> So, I still plan to separate the patch set into 2 parts:
> >>>
> >>>   Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
> >>>                  Then kernel can use __FORTIFY_SOURCE correctly;
> >>>
> >>>   Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
> >>>
> >>> Let me know if you have any comment and suggestion.
> >>
> >> It might be worth considering whether it should be possible to control
> >> the "flexible array" property separately for each trailing array member
> >> via either a #pragma or an attribute in headers that can't change
> >> the struct layout but that need to be usable in programs compiled with
> >> stricter -fstrict-flex-array=N settings.
> >
> > Or an decl attribute.
>
> Yes, it might be necessary to add a corresponding decl attribute
>
> strict_flex_array (N)
>
> Which is attached to a trailing structure array member to provide the user a finer control when -fstrict-flex-array=N is specified.
>
> So, I will do the following:
>
>
> *****User interface:
>
> 1. command line option:
>      -fstrict-flex-array=N       (N=0, 1, 2, 3)
> 2.  decl attribute:
>      strict_flex_array (N)      (N=0, 1, 2, 3)
>
>
> *****Implementation:
>
> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
>      [], the option -fstrict-flex-array, the attribute strict_flex_array,  and whether it’s the last field
>      of the DECL_CONTEXT.
> 3. In Middle end,   update all users of “array_at_struct_end_p" or “component_ref_size”, or any place that treats
>     Trailing array as flexible array member with the new flag  DECL_NOT_FLEXARRAY.
>     (Still think we need a new consistent utility routine here).
>
>
> I still plan to separate the patch set into 2 parts:
>
> Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
>                Then kernel can use __FORTIFY_SOURCE correctly.
> Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
>
>
> Let me know any more comment or suggestion.

Sounds good.  Part 3. is "optimization" and reasonable to do
separately, I'm not sure you need
'B' (since we're not supposed to have new utilities), but instead I'd
do '3.' as part of 'B', just
changing the pieces th resolve PR101836 for part 'A'.

Richard.

> Thanks a lot.
>
> Qing
>
>

Re: [GCC 13][PATCH] PR101836: Add a new option -fstrict-flex-array[=n] and use it in __builtin_object_size

[Qing Zhao]

> On Jul 7, 2022, at 4:02 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Wed, Jul 6, 2022 at 4:20 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>> 
>> (Sorry for the late reply, just came back from a short vacation.)
>> 
>>> On Jul 4, 2022, at 2:49 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>> 
>>> On Fri, Jul 1, 2022 at 5:32 PM Martin Sebor <msebor@gmail.com> wrote:
>>>> 
>>>> On 7/1/22 08:01, Qing Zhao wrote:
>>>>> 
>>>>> 
>>>>>> On Jul 1, 2022, at 8:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>>> 
>>>>>> On Fri, Jul 01, 2022 at 12:55:08PM +0000, Qing Zhao wrote:
>>>>>>> If so, comparing to the current implemenation to have all the checking in middle-end, what’s the
>>>>>>> major benefit of moving part of the checking into FE, and leaving the other part in middle-end?
>>>>>> 
>>>>>> The point is recording early what FIELD_DECLs could be vs. can't possibly be
>>>>>> treated like flexible array members and just use that flag in the decisions
>>>>>> in the current routines in addition to what it is doing.
>>>>> 
>>>>> Okay.
>>>>> 
>>>>> Based on the discussion so far, I will do the following:
>>>>> 
>>>>> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
>>>>> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
>>>>>    [] and the option -fstrict-flex-array, and whether it’s the last field of the DECL_CONTEXT.
>>>>> 3. In Middle end,  Add a new utility routine is_flexible_array_member_p, which bases on
>>>>>    DECL_NOT_FLEXARRAY + array_at_struct_end_p to decide whether the array
>>>>>    reference is a real flexible array member reference.
>>> 
>>> I would just update all existing users, not introduce another wrapper
>>> that takes DECL_NOT_FLEXARRAY
>>> into account additionally.
>> 
>> Okay.
>>> 
>>>>> 
>>>>> 
>>>>> Middle end currently is quite mess, array_at_struct_end_p, component_ref_size, and all the phases that
>>>>> use these routines need to be updated, + new testing cases for each of the phases.
>>>>> 
>>>>> 
>>>>> So, I still plan to separate the patch set into 2 parts:
>>>>> 
>>>>>  Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
>>>>>                 Then kernel can use __FORTIFY_SOURCE correctly;
>>>>> 
>>>>>  Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
>>>>> 
>>>>> Let me know if you have any comment and suggestion.
>>>> 
>>>> It might be worth considering whether it should be possible to control
>>>> the "flexible array" property separately for each trailing array member
>>>> via either a #pragma or an attribute in headers that can't change
>>>> the struct layout but that need to be usable in programs compiled with
>>>> stricter -fstrict-flex-array=N settings.
>>> 
>>> Or an decl attribute.
>> 
>> Yes, it might be necessary to add a corresponding decl attribute
>> 
>> strict_flex_array (N)
>> 
>> Which is attached to a trailing structure array member to provide the user a finer control when -fstrict-flex-array=N is specified.
>> 
>> So, I will do the following:
>> 
>> 
>> *****User interface:
>> 
>> 1. command line option:
>>     -fstrict-flex-array=N       (N=0, 1, 2, 3)
>> 2.  decl attribute:
>>     strict_flex_array (N)      (N=0, 1, 2, 3)
>> 
>> 
>> *****Implementation:
>> 
>> 1. Add a new flag “DECL_NOT_FLEXARRAY” to FIELD_DECL;
>> 2. In C/C++ FE, set the new flag “DECL_NOT_FLEXARRAY” for a FIELD_DECL based on [0], [1],
>>     [], the option -fstrict-flex-array, the attribute strict_flex_array,  and whether it’s the last field
>>     of the DECL_CONTEXT.
>> 3. In Middle end,   update all users of “array_at_struct_end_p" or “component_ref_size”, or any place that treats
>>    Trailing array as flexible array member with the new flag  DECL_NOT_FLEXARRAY.
>>    (Still think we need a new consistent utility routine here).
>> 
>> 
>> I still plan to separate the patch set into 2 parts:
>> 
>> Part A:    the above 1 + 2 + 3,  and use these new utilities in tree-object-size.cc to resolve PR101836 first.
>>               Then kernel can use __FORTIFY_SOURCE correctly.
>> Part B:    update all other phases with the new utilities + new testing cases + resolving regressions.
>> 
>> 
>> Let me know any more comment or suggestion.
> 
> Sounds good.  Part 3. is "optimization" and reasonable to do
> separately, I'm not sure you need
> 'B' (since we're not supposed to have new utilities), but instead I'd
> do '3.' as part of 'B', just
> changing the pieces th resolve PR101836 for part 'A'.


Okay, I see. Then I will separate the patches to:

Part A:   1 + 2
Part B:  In Middle end, use the new flag in tree-object-size.cc to resolve PR101836, then kernel can use __FORTIFY_SOURCE correctly after this;
Part C:  in Middle end, use the new flag in all other places that use “array_at_struct_end_p” or “component_ref_size” to make GCC consistently 
             behave for trailing array. 

The reason I separate the middle end work into Part B and C is: Part B is immediately required by PR101836;  Part C is needed to fix all other GCC
phases to  treat trailing array consistently, it’s not needed for PR101836, but it’s important to make GCC to consistently behave on trailing arrays. 

Qing