diff mbox

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

Message ID 780797da-9742-2f9b-dbc0-4209a9176391@gmail.com
State New
Headers show

Commit Message

Martin Sebor Oct. 21, 2016, 11:47 p.m. UTC
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

Comments

Eric Botcazou Oct. 22, 2016, 9:37 a.m. UTC | #1
> 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
Jeff Law Oct. 24, 2016, 3:13 p.m. UTC | #2
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  <msebor@redhat.com>

>

> 	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  <msebor@redhat.com>

>

> 	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
Martin Sebor Oct. 30, 2016, 10:48 p.m. UTC | #3
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  <msebor@redhat.com>

>>

>>     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  <msebor@redhat.com>

>>

>>     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
Jason Merrill Oct. 31, 2016, 1:27 p.m. UTC | #4
OK.

Jason
diff mbox

Patch

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

gcc/cp/ChangeLog:
2016-10-21  Martin Sebor  <msebor@redhat.com>

	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  <msebor@redhat.com>

	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, a1[]; } s1;
+  struct { double n2, a2[]; } s2;
+  char n3;
+};
+
+union UnionStruct4 {
+  struct { int n1, a1[]; } s1;
+  struct { struct { int n2, a2[]; } s2; } s3;
+  char n3;
+};
+
+union UnionStruct5 {
+  struct { struct { int n1, a1[]; } s1; } s2;   // { dg-warning "invalid use" }
+  struct { double n2, a2[]; } s3;
+  char n3;
+};
+
+union UnionStruct6 {
+  struct { struct { int n1, a1[]; } s1; } s2;   // { dg-warning "invalid use" }
+  struct { struct { int n2, a2[]; } s3; } s4;
+  char n3;
+};
+
+union UnionStruct7 {
+  struct { int n1, a1[]; } s1;
+  struct { double n2, a2[]; } s2;
+  struct { struct { int n3, a3[]; } s3; } s4;
+};
+
+union UnionStruct8 {
+  struct { int n1, a1[]; } s1;
+  struct { struct { int n2, a2[]; } s2; } s3;
+  struct { struct { int n3, a3[]; } s4; } s5;
+};
+
+union UnionStruct9 {
+  struct { struct { int n1, a1[]; } s1; } s2;   // { dg-warning "invalid use" }
+  struct { struct { int n2, a2[]; } s3; } s4;
+  struct { struct { int n3, a3[]; } s5; } s6;
+};
+
+struct StructUnion1 {
+  union {
+    struct { int n1, a1[]; } s1;        // { dg-message "declared here" }
+    struct { double n2, a2[]; } s2;
+    char n3;
+  } u;                                  // { dg-warning "invalid use" }
+};
+
+// The following are invalid and rejected.
+struct StructUnion2 {
+  union {
+    struct { int n1, a1[]; } s1;        // { dg-warning "not at end" }
+  } u;
+  char n3;                              // { dg-message "next member" }
+};
+
+struct StructUnion3 {
+  union {
+    struct { int n1, a1[]; } s1;        // { dg-warning "not at end" }
+    struct { double n2, a2[]; } s2;
+  } u;
+  char n3;                              // { dg-message "next member" }
+};
+
+struct StructUnion4 {
+  union {
+    struct { int n1, a1[]; } s1;        // { dg-warning "not at end" }
+  } u1;
+  union {
+    struct { double n2, a2[]; } s2;
+  } u2;                                 // { dg-message "next member" }
+};
+
+struct StructUnion5 {
+  union {
+    union {
+      struct { int n1, a1[]; } s1;      // { dg-message "declared here" }
+    } u1;
+    union { struct { int n2, a2[]; } s2; } u2;
+  } u;                                  // { dg-warning "invalid use" }
+};
+
+struct StructUnion6 {
+  union {
+    struct { int n1, a1[]; } s1;        // { dg-message "declared here" }
+    union { struct { int n2, a2[]; } s2; } u2;
+  } u;                                  // { dg-warning "invalid use" }
+};
+
+struct StructUnion7 {
+  union {
+    union {
+      struct { double n2, a2[]; } s2;   // { dg-message "declared here" }
+    } u2;
+    struct { int n1, a1[]; } s1;
+  } u;                                  // { dg-warning "invalid use" }
+};
+
+struct StructUnion8 {
+  struct {
+    union {
+      union {
+	struct { int n1, a1[]; } s1;    // { dg-warning "not at end" }
+      } u1;
+      union {
+	struct { double n2, a2[]; } s2;
+      } u2;
+    } u;
+  } s1;
+
+  struct {
+    union {
+      union {
+	struct { int n1, a1[]; } s1;
+      } u1;
+      union {
+	struct { double n2, a2[]; } s2;
+      } u2;
+    } u; } s2;                              // { dg-message "next member" }
+};
+
+struct StructUnion9 {                       // { dg-message "in the definition" }
+  struct A1 {
+    union B1 {
+      union C1 {
+	struct Sx1 { int n1, a1[]; } sx1;   // { dg-warning "not at end" }
+      } c1;
+      union D1 {
+	struct Sx2 { double n2, a2[]; } sx2;
+      } d1;
+    } b1;                                   // { dg-warning "invalid use" }
+  } a1;
+
+  struct A2 {
+    union B2 {
+      union C2 {
+	struct Sx3 { int n3, a3[]; } sx3;   // { dg-message "declared here" }
+      } c2;
+      union D2 { struct Sx4 { double n4, a4[]; } sx4; } d2;
+    } b2;                                   // { dg-warning "invalid use" }
+  } a2;                                     // { dg-message "next member" }
+};
Index: gcc/testsuite/g++.dg/ext/flexary19.C
===================================================================
--- gcc/testsuite/g++.dg/ext/flexary19.C	(revision 0)
+++ gcc/testsuite/g++.dg/ext/flexary19.C	(working copy)
@@ -0,0 +1,343 @@ 
+// { dg-do compile }
+// { dg-additional-options "-Wpedantic -Wno-error=pedantic" }
+
+// Verify that flexible array members are recognized as either valid
+// or invalid in anonymous structs (a G++ extension) and C++ anonymous
+// unions as well as in structs and unions that look anonymous but
+// aren't.
+struct S1
+{
+  int i;
+
+  // The following declares a named data member of an unnamed struct
+  // (i.e., it is not an anonymous struct).
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s;
+};
+
+struct S2
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s[1];
+};
+
+struct S3
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s[];
+};
+
+struct S4
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s[2];
+};
+
+struct S5
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s[1][2];
+};
+
+struct S6
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s[][2];
+};
+
+struct S7
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } *s;
+};
+
+struct S8
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } **s;
+};
+
+struct S9
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } *s[1];
+};
+
+struct S10
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } *s[];
+};
+
+struct S11
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } **s[1];
+};
+
+struct S12
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } **s[];
+};
+
+struct S13
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } **s[2];
+};
+
+struct S14
+{
+  int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } &s;
+};
+
+struct S15
+{
+  int i;
+
+  typedef struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } T15;
+};
+
+struct S16
+{
+  int i;
+
+  struct {          // { dg-warning "invalid use" }
+    // A flexible array as a sole member of an anonymous struct is
+    // rejected with an error in C mode but emits just a pedantic
+    // warning in C++.  Other than excessive pedantry there is no
+    // reason to reject it.
+    int a[];
+  };                // { dg-warning "anonymous struct" }
+};
+
+struct S17
+{
+  int i;
+
+  union {           // anonymous union
+    int a[];        // { dg-error "flexible array member in union" }
+  };
+};
+
+struct S18
+{
+  int i;
+
+  struct {
+    int j, a[];     // { dg-message "declared here" }
+  } s;              // { dg-warning "invalid use" }
+};
+
+struct S19
+{
+  int i;
+
+  struct {          // { dg-warning "invalid use" }
+    int j, a[];     // { dg-message "declared here" }
+  };                // { dg-warning "anonymous struct" }
+};
+
+struct S20
+{
+  static int i;
+  typedef int A[];
+
+  struct {
+    int j;
+    A a;            // { dg-message "declared here" }
+  } s;              // { dg-warning "invalid use" }
+};
+
+struct S21
+{
+  static int i;
+  typedef int A[];
+
+  struct {          // { dg-warning "invalid use" }
+    int j;
+    A a;            // { dg-message "declared here" }
+  };                // { dg-warning "anonymous struct" }
+};
+
+struct S22
+{
+  struct S22S {
+    static int i;
+
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s;
+};
+
+struct S23
+{
+  struct {
+    static int i;   // { dg-error "static data member" }
+
+    int a[];        // { dg-error "in an otherwise empty" }
+  };                // { dg-warning "anonymous struct" }
+};
+
+struct S24
+{
+  static int i;
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s;
+};
+
+struct S25
+{
+  int i;
+
+  struct {
+    int j, a[];     // { dg-message "declared here" }
+  } s;              // { dg-warning "invalid use" }
+
+  // Verify that a static data member of the enclosing class doesn't
+  // cause infinite recursion or some such badness.
+  static S25 s2;
+};
+
+struct S26
+{
+  template <class>
+  struct S26S {
+    static int a;
+  };
+
+  struct {
+    int a[];        // { dg-error "in an otherwise empty" }
+  } s;
+};
+
+struct S27
+{
+  S27 *p;
+  int a[];
+};
+
+struct S28
+{
+  struct A {
+    struct B {
+      S28 *ps28;
+      A   *pa;
+      B   *pb;
+    } b, *pb;
+    A *pa;
+  } a, *pa;
+
+  S28::A *pa2;
+  S28::A::B *pb;
+
+  int flexarray[];
+};
+
+// Verify that the notes printed along with the warnings point to the types
+// or members they should point to and mention the correct relationships
+// with the flexible array members.
+namespace Notes
+{
+union A
+{
+  struct {
+    struct {
+      int i, a[];   // { dg-message "declared here" }
+    } c;            // { dg-warning "invalid use" }
+  } d;
+  int j;
+};
+
+union B
+{
+  struct {
+    struct {        // { dg-warning "invalid use" }
+      int i, a[];   // { dg-message "declared here" }
+    };              // { dg-warning "anonymous struct" }
+  };                // { dg-warning "anonymous struct" }
+  int j;
+};
+
+}
+
+typedef struct Opaque* P29;
+struct S30 { P29 p; };
+struct S31 { S30 s; };
+
+typedef struct { } S32;
+typedef struct { S32 *ps32; } S33;
+typedef struct
+{
+  S33 *ps33;
+} S34;
+
+struct S35
+{
+  struct A {
+    int i1, a1[];
+  };
+
+  struct B {
+    int i2, a2[];
+  };
+
+  typedef struct {
+    int i3, a3[];
+  } C;
+
+  typedef struct {
+    int i4, a4[];
+  } D;
+
+  typedef A A2;
+  typedef B B2;
+  typedef C C2;
+  typedef D D2;
+};
+