Re: relax rule for flexible array members in 6.x (78039 - fails to compile glibc tests)
OK. Jason
Re: relax rule for flexible array members in 6.x (78039 - fails to compile glibc tests)
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)
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)
> 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)
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, a1[]; } s1; + struct { double n2, a2[];