Re: relax rule for flexible array members in 6.x (78039 - fails to compile glibc tests)

[Jason Merrill]

OK.

Jason

Re: relax rule for flexible array members in 6.x (78039 - fails to compile glibc tests)

[Martin Sebor]

Thanks Jeff.  I'll take care of the nits before I commit the patch
and update the Web page this week.

Jason, assuming you agree that the checking should be relaxed for
6.0, can you please let me know if it's good to commit?

Martin

On 10/24/2016 09:13 AM, Jeff Law wrote:

On 10/21/2016 05:47 PM, Martin Sebor wrote:

Bug 78039 complains that the fix for c++/71912 recently backported
to the GCC 6 branch causes GCC 6 to reject Glibc tests that expect
to be able to define structs with multiple flexible array members,
despite it violating the C standard(*).

The rejected code is unsafe and was intended to be rejected in 6.1
to begin with (i.e., it was a bug I had missed that the code wasn't
rejected in 6.1), and an alternate solution exists, so the backport
seemed appropriate to me.

However, it was pointed out to me that apparently there is a policy
or convention of not backporting to release branches bug fixes that
cause GCC to reject code that was previously accepted, even if the
code is invalid.

To comply with this policy the attached patch adjusts the backported
code to accept the invalid flexible array member with just a pedantic
warning (same as in C mode).  The patch also adds the tests that were
part of the fix for bug 71912 but that were accidentally left out of
the original backport.

Martin

[*] Bug 77650 discusses the background on this.

PS I checked the GCC Development Plan but couldn't find a mention
of this policy.  Since this seems like an important guarantee for
users to know about and for contributors to maintain I suggest to
update the document to reflect it.  If there is are no objections
I'll propose a separate change to mention it.

  https://gcc.gnu.org/develop.html

gcc-78039.diff


PR c++/78039 - fails to compile glibc tests

gcc/cp/ChangeLog:
2016-10-21  Martin Sebor  

PR c++/78039
* class.c (diagnose_flexarrays): Avoid rejecting an invalid flexible
array member with a hard error when it is followed by anbother member

s/anbother/another/


in a different struct, and instead issue just a pedantic warning.

gcc/testsuite/ChangeLog:
2016-10-21  Martin Sebor  

PR c++/78039
* g++.dg/ext/flexary18.C: New test.
* g++.dg/ext/flexary19.C: New test.

Index: gcc/cp/class.c
===
--- gcc/cp/class.c(revision 241433)
+++ gcc/cp/class.c(working copy)
@@ -6960,7 +6960,20 @@ diagnose_flexarrays (tree t, const flexmems_t *fme
   location_t loc = DECL_SOURCE_LOCATION (fmem->array);
   diagd = true;

-  error_at (loc, msg, fmem->array, t);
+  /* For compatibility with GCC 6.2 and 6.1 reject with an error
+ a flexible array member of a plain struct that's followed
+ by another member only if they are both members of the same
+ struct.  Otherwise, issue just a pedantic warning.  See bug
+ 71375 for details.  */

71375?  That bug looks totally unrelated.  Did you mean 71912?



Jason should have final call on the C++ bits.  But figured I'd point out
the nits.

As far as updating the web page to mention the caveat about this aspect
of the backporting policy, please do.

Jeff

Re: relax rule for flexible array members in 6.x (78039 - fails to compile glibc tests)

[Jeff Law]

On 10/21/2016 05:47 PM, Martin Sebor wrote:

Bug 78039 complains that the fix for c++/71912 recently backported
to the GCC 6 branch causes GCC 6 to reject Glibc tests that expect
to be able to define structs with multiple flexible array members,
despite it violating the C standard(*).

The rejected code is unsafe and was intended to be rejected in 6.1
to begin with (i.e., it was a bug I had missed that the code wasn't
rejected in 6.1), and an alternate solution exists, so the backport
seemed appropriate to me.

However, it was pointed out to me that apparently there is a policy
or convention of not backporting to release branches bug fixes that
cause GCC to reject code that was previously accepted, even if the
code is invalid.

To comply with this policy the attached patch adjusts the backported
code to accept the invalid flexible array member with just a pedantic
warning (same as in C mode).  The patch also adds the tests that were
part of the fix for bug 71912 but that were accidentally left out of
the original backport.

Martin

[*] Bug 77650 discusses the background on this.

PS I checked the GCC Development Plan but couldn't find a mention
of this policy.  Since this seems like an important guarantee for
users to know about and for contributors to maintain I suggest to
update the document to reflect it.  If there is are no objections
I'll propose a separate change to mention it.

  https://gcc.gnu.org/develop.html

gcc-78039.diff


PR c++/78039 - fails to compile glibc tests

gcc/cp/ChangeLog:
2016-10-21  Martin Sebor  

PR c++/78039
* class.c (diagnose_flexarrays): Avoid rejecting an invalid flexible
array member with a hard error when it is followed by anbother member

s/anbother/another/


in a different struct, and instead issue just a pedantic warning.

gcc/testsuite/ChangeLog:
2016-10-21  Martin Sebor  

PR c++/78039
* g++.dg/ext/flexary18.C: New test.
* g++.dg/ext/flexary19.C: New test.

Index: gcc/cp/class.c
===
--- gcc/cp/class.c  (revision 241433)
+++ gcc/cp/class.c  (working copy)
@@ -6960,7 +6960,20 @@ diagnose_flexarrays (tree t, const flexmems_t *fme
  location_t loc = DECL_SOURCE_LOCATION (fmem->array);
  diagd = true;

- error_at (loc, msg, fmem->array, t);
+ /* For compatibility with GCC 6.2 and 6.1 reject with an error
+a flexible array member of a plain struct that's followed
+by another member only if they are both members of the same
+struct.  Otherwise, issue just a pedantic warning.  See bug
+71375 for details.  */

71375?  That bug looks totally unrelated.  Did you mean 71912?



Jason should have final call on the C++ bits.  But figured I'd point out 
the nits.


As far as updating the web page to mention the caveat about this aspect 
of the backporting policy, please do.


Jeff

Re: relax rule for flexible array members in 6.x (78039 - fails to compile glibc tests)

[Eric Botcazou]

> However, it was pointed out to me that apparently there is a policy
> or convention of not backporting to release branches bug fixes that
> cause GCC to reject code that was previously accepted, even if the
> code is invalid.

It's more of a judgment call I'd say, if the accept-invalid leads to wrong 
code in all cases, then you want to plug the hole; if it's benign in almost 
all cases, then it's a different discussion.

-- 
Eric Botcazou

relax rule for flexible array members in 6.x (78039 - fails to compile glibc tests)

[Martin Sebor]

Bug 78039 complains that the fix for c++/71912 recently backported
to the GCC 6 branch causes GCC 6 to reject Glibc tests that expect
to be able to define structs with multiple flexible array members,
despite it violating the C standard(*).

The rejected code is unsafe and was intended to be rejected in 6.1
to begin with (i.e., it was a bug I had missed that the code wasn't
rejected in 6.1), and an alternate solution exists, so the backport
seemed appropriate to me.

However, it was pointed out to me that apparently there is a policy
or convention of not backporting to release branches bug fixes that
cause GCC to reject code that was previously accepted, even if the
code is invalid.

To comply with this policy the attached patch adjusts the backported
code to accept the invalid flexible array member with just a pedantic
warning (same as in C mode).  The patch also adds the tests that were
part of the fix for bug 71912 but that were accidentally left out of
the original backport.

Martin

[*] Bug 77650 discusses the background on this.

PS I checked the GCC Development Plan but couldn't find a mention
of this policy.  Since this seems like an important guarantee for
users to know about and for contributors to maintain I suggest to
update the document to reflect it.  If there is are no objections
I'll propose a separate change to mention it.

  https://gcc.gnu.org/develop.html
PR c++/78039 - fails to compile glibc tests

gcc/cp/ChangeLog:
2016-10-21  Martin Sebor  

	PR c++/78039
	* class.c (diagnose_flexarrays): Avoid rejecting an invalid flexible
	array member with a hard error when it is followed by anbother member
	in a different struct, and instead issue just a pedantic warning.

gcc/testsuite/ChangeLog:
2016-10-21  Martin Sebor  

	PR c++/78039
	* g++.dg/ext/flexary18.C: New test.
	* g++.dg/ext/flexary19.C: New test.

Index: gcc/cp/class.c
===
--- gcc/cp/class.c	(revision 241433)
+++ gcc/cp/class.c	(working copy)
@@ -6960,7 +6960,20 @@ diagnose_flexarrays (tree t, const flexmems_t *fme
 	  location_t loc = DECL_SOURCE_LOCATION (fmem->array);
 	  diagd = true;
 
-	  error_at (loc, msg, fmem->array, t);
+	  /* For compatibility with GCC 6.2 and 6.1 reject with an error
+	 a flexible array member of a plain struct that's followed
+	 by another member only if they are both members of the same
+	 struct.  Otherwise, issue just a pedantic warning.  See bug
+	 71375 for details.  */
+	  if (fmem->after[0]
+	  && (!TYPE_BINFO (t)
+		  || 0 == BINFO_N_BASE_BINFOS (TYPE_BINFO (t)))
+	  && DECL_CONTEXT (fmem->array) != DECL_CONTEXT (fmem->after[0])
+	  && !ANON_AGGR_TYPE_P (DECL_CONTEXT (fmem->array))
+	  && !ANON_AGGR_TYPE_P (DECL_CONTEXT (fmem->after[0])))
+	pedwarn (loc, OPT_Wpedantic, msg, fmem->array, t);
+	  else
+	error_at (loc, msg, fmem->array, t);
 
 	  /* In the unlikely event that the member following the flexible
 	 array member is declared in a different class, or the member
Index: gcc/testsuite/g++.dg/ext/flexary18.C
===
--- gcc/testsuite/g++.dg/ext/flexary18.C	(revision 0)
+++ gcc/testsuite/g++.dg/ext/flexary18.C	(working copy)
@@ -0,0 +1,213 @@
+// PR c++/71912 - [6/7 regression] flexible array in struct in union rejected
+// { dg-do compile }
+// { dg-additional-options "-Wpedantic -Wno-error=pedantic" }
+
+#if __cplusplus
+
+namespace pr71912 {
+
+#endif
+
+struct foo {
+  int a;
+  char s[]; // { dg-message "array member .char pr71912::foo::s \\\[\\\]. declared here" }
+};
+
+struct bar {
+  double d;
+  char t[];
+};
+
+struct baz {
+  union {
+struct foo f;
+struct bar b;
+  }
+  // The definition of struct foo is fine but the use of struct foo
+  // in the definition of u below is what's invalid and must be clearly
+  // diagnosed.
+u;  // { dg-warning "invalid use of .struct pr71912::foo. with a flexible array member in .struct pr71912::baz." }
+};
+
+struct xyyzy {
+  union {
+struct {
+  int a;
+  char s[]; // { dg-message "declared here" }
+} f;
+struct {
+  double d;
+  char t[];
+} b;
+  } u;  // { dg-warning "invalid use" }
+};
+
+struct baz b;
+struct xyyzy x;
+
+#if __cplusplus
+
+}
+
+#endif
+
+// The following definitions aren't strictly valid but, like those above,
+// are accepted for compatibility with GCC (in C mode).  They are benign
+// in that the flexible array member is at the highest offset within
+// the outermost type and doesn't overlap with other members except for
+// those of the union.
+union UnionStruct1 {
+  struct { int n1, a[]; } s;
+  int n2;
+};
+
+union UnionStruct2 {
+  struct { int n1, a1[]; } s1;
+  struct { int n2, a2[]; } s2;
+  int n3;
+};
+
+union UnionStruct3 {
+  struct { int n1,