| Message ID | 4c6d8cdd-46f8-216d-56cb-07a74284f69a@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 20, 2017 at 7:04 PM, Martin Sebor <msebor@gmail.com> wrote: > Attached is a minimal patch to avoid an ICE in CHKP upon > encountering one form of an initializer for a flexible array > member, specifically the empty string: > > int f () > { > struct B { int n; char a[]; }; > > return ((struct B){ 1, "" }).a[0]; > } > > Although GCC accepts (and doesn't ICE on) non-empty initializers > for flexible array members, such as > > (struct B){ 1, "123" } How do you mean? When I compile this with the C front end, I get error: non-static initialization of a flexible array member Jason
On 03/20/2017 05:51 PM, Jason Merrill wrote: > On Mon, Mar 20, 2017 at 7:04 PM, Martin Sebor <msebor@gmail.com> wrote: >> Attached is a minimal patch to avoid an ICE in CHKP upon >> encountering one form of an initializer for a flexible array >> member, specifically the empty string: >> >> int f () >> { >> struct B { int n; char a[]; }; >> >> return ((struct B){ 1, "" }).a[0]; >> } >> >> Although GCC accepts (and doesn't ICE on) non-empty initializers >> for flexible array members, such as >> >> (struct B){ 1, "123" } > > How do you mean? When I compile this with the C front end, I get > > error: non-static initialization of a flexible array member I meant that G++ accepts it, doesn't ICE, but emits wrong code. (it's consistently rejected by the C front end). Sorry for not being clear about it. Martin
On Mon, Mar 20, 2017 at 7:58 PM, Martin Sebor <msebor@gmail.com> wrote: > On 03/20/2017 05:51 PM, Jason Merrill wrote: >> On Mon, Mar 20, 2017 at 7:04 PM, Martin Sebor <msebor@gmail.com> wrote: >>> >>> Attached is a minimal patch to avoid an ICE in CHKP upon >>> encountering one form of an initializer for a flexible array >>> member, specifically the empty string: >>> >>> int f () >>> { >>> struct B { int n; char a[]; }; >>> >>> return ((struct B){ 1, "" }).a[0]; >>> } >>> >>> Although GCC accepts (and doesn't ICE on) non-empty initializers >>> for flexible array members, such as >>> >>> (struct B){ 1, "123" } >> >> >> How do you mean? When I compile this with the C front end, I get >> >> error: non-static initialization of a flexible array member > > I meant that G++ accepts it, doesn't ICE, but emits wrong code. > (it's consistently rejected by the C front end). Sorry for not > being clear about it. Ah, OK. It seems to me that the wrong code bug is worth addressing; why does rejecting the code seem risky to you? Jason
On 03/20/2017 10:27 PM, Jason Merrill wrote: > On Mon, Mar 20, 2017 at 7:58 PM, Martin Sebor <msebor@gmail.com> wrote: >> On 03/20/2017 05:51 PM, Jason Merrill wrote: >>> On Mon, Mar 20, 2017 at 7:04 PM, Martin Sebor <msebor@gmail.com> wrote: >>>> >>>> Attached is a minimal patch to avoid an ICE in CHKP upon >>>> encountering one form of an initializer for a flexible array >>>> member, specifically the empty string: >>>> >>>> int f () >>>> { >>>> struct B { int n; char a[]; }; >>>> >>>> return ((struct B){ 1, "" }).a[0]; >>>> } >>>> >>>> Although GCC accepts (and doesn't ICE on) non-empty initializers >>>> for flexible array members, such as >>>> >>>> (struct B){ 1, "123" } >>> >>> >>> How do you mean? When I compile this with the C front end, I get >>> >>> error: non-static initialization of a flexible array member >> >> I meant that G++ accepts it, doesn't ICE, but emits wrong code. >> (it's consistently rejected by the C front end). Sorry for not >> being clear about it. > > Ah, OK. It seems to me that the wrong code bug is worth addressing; > why does rejecting the code seem risky to you? I have a few reasons: First, it's not a regression (I raised bug 69696 for it in early 2016) so strictly it's out of scope for this stage. Second, there are a number of bugs related to the initialization of flexible array members so the fixes are probably not going to be contained to a single function or file. Third, the flexible member array changes I made in the past were not trivial, took time to develop, and the two of us iterated over some of them for weeks. Despite your careful review and my extensive testing some of them introduced regressions that are still being patched up. Fourth, making a change to reject code this close to a release runs the risk of breaking code that has successfully compiled in mass rebuilds and others' tests with the new compiler. While that could be viewed as a good change for invalid code that's exercised at run time, it could also break programs where the bad code is never exercised. As I understand the schedule, the release is expected sometime in early April. I leave on April 2 for a week, so I have only until then. I don't think that leaves enough time. I'd be uncomfortable taking on a project this late in the cycle that could put the release in jeopardy, or that I might have to rely on others to finish up. Martin
On Tue, Mar 21, 2017 at 09:08:49AM -0600, Martin Sebor wrote: > As I understand the schedule, the release is expected sometime > in early April. I leave on April 2 for a week, so I have only > until then. I don't think that leaves enough time. I'd be > uncomfortable taking on a project this late in the cycle that > could put the release in jeopardy, or that I might have to > rely on others to finish up. The release is expected when it is ready. We still have 8 P1s, so we don't meet the release criteria, and have way too many P2-P3s, early April is very unlikely, late April is much more likely than that. Jakub
On 03/21/2017 09:15 AM, Jakub Jelinek wrote: > On Tue, Mar 21, 2017 at 09:08:49AM -0600, Martin Sebor wrote: >> As I understand the schedule, the release is expected sometime >> in early April. I leave on April 2 for a week, so I have only >> until then. I don't think that leaves enough time. I'd be >> uncomfortable taking on a project this late in the cycle that >> could put the release in jeopardy, or that I might have to >> rely on others to finish up. > > The release is expected when it is ready. We still have 8 P1s, > so we don't meet the release criteria, and have way too many P2-P3s, > early April is very unlikely, late April is much more likely than that. Late April is consistent with my estimate of when the release will be ready as well. jeff
On Tue, Mar 21, 2017 at 09:08:49AM -0600, Martin Sebor wrote: > On 03/20/2017 10:27 PM, Jason Merrill wrote: > > On Mon, Mar 20, 2017 at 7:58 PM, Martin Sebor <msebor@gmail.com> wrote: > > > On 03/20/2017 05:51 PM, Jason Merrill wrote: > > > > On Mon, Mar 20, 2017 at 7:04 PM, Martin Sebor <msebor@gmail.com> wrote: > > > > > > > > > > Attached is a minimal patch to avoid an ICE in CHKP upon > > > > > encountering one form of an initializer for a flexible array > > > > > member, specifically the empty string: > > > > > > > > > > int f () > > > > > { > > > > > struct B { int n; char a[]; }; > > > > > > > > > > return ((struct B){ 1, "" }).a[0]; > > > > > } > > > > > > > > > > Although GCC accepts (and doesn't ICE on) non-empty initializers > > > > > for flexible array members, such as > > > > > > > > > > (struct B){ 1, "123" } > > > > > > > > > > > > How do you mean? When I compile this with the C front end, I get > > > > > > > > error: non-static initialization of a flexible array member > > > > > > I meant that G++ accepts it, doesn't ICE, but emits wrong code. > > > (it's consistently rejected by the C front end). Sorry for not > > > being clear about it. > > > > Ah, OK. It seems to me that the wrong code bug is worth addressing; > > why does rejecting the code seem risky to you? > > I have a few reasons: First, it's not a regression (I raised > bug 69696 for it in early 2016) so strictly it's out of scope > for this stage. Second, there are a number of bugs related > to the initialization of flexible array members so the fixes > are probably not going to be contained to a single function > or file. Third, the flexible member array changes I made in > the past were not trivial, took time to develop, and the two > of us iterated over some of them for weeks. Despite your > careful review and my extensive testing some of them > introduced regressions that are still being patched up. > Fourth, making a change to reject code this close to a release > runs the risk of breaking code that has successfully compiled > in mass rebuilds and others' tests with the new compiler. > While that could be viewed as a good change for invalid code > that's exercised at run time, it could also break programs > where the bad code is never exercised. > > As I understand the schedule, the release is expected sometime > in early April. I leave on April 2 for a week, so I have only > until then. I don't think that leaves enough time. I'd be > uncomfortable taking on a project this late in the cycle that > could put the release in jeopardy, or that I might have to > rely on others to finish up. Since I've also spent some time on this: my take on this is that the C++ FE should just follow C FE's suit and reject such initializations where possible; it seems they've never worked reliably anyway, and bring more harm than good. I don't see that rejecting such code would cause too much trouble, if any, although the timing could be better. Marek
On Tue, Mar 21, 2017 at 11:08 AM, Martin Sebor <msebor@gmail.com> wrote: > On 03/20/2017 10:27 PM, Jason Merrill wrote: >> >> On Mon, Mar 20, 2017 at 7:58 PM, Martin Sebor <msebor@gmail.com> wrote: >>> >>> On 03/20/2017 05:51 PM, Jason Merrill wrote: >>>> >>>> On Mon, Mar 20, 2017 at 7:04 PM, Martin Sebor <msebor@gmail.com> wrote: >>>>> >>>>> >>>>> Attached is a minimal patch to avoid an ICE in CHKP upon >>>>> encountering one form of an initializer for a flexible array >>>>> member, specifically the empty string: >>>>> >>>>> int f () >>>>> { >>>>> struct B { int n; char a[]; }; >>>>> >>>>> return ((struct B){ 1, "" }).a[0]; >>>>> } >>>>> >>>>> Although GCC accepts (and doesn't ICE on) non-empty initializers >>>>> for flexible array members, such as >>>>> >>>>> (struct B){ 1, "123" } >>>> >>>> How do you mean? When I compile this with the C front end, I get >>>> >>>> error: non-static initialization of a flexible array member >>> >>> I meant that G++ accepts it, doesn't ICE, but emits wrong code. >>> (it's consistently rejected by the C front end). Sorry for not >>> being clear about it. >> >> Ah, OK. It seems to me that the wrong code bug is worth addressing; >> why does rejecting the code seem risky to you? > > I have a few reasons: First, it's not a regression (I raised > bug 69696 for it in early 2016) so strictly it's out of scope > for this stage. Second, there are a number of bugs related > to the initialization of flexible array members so the fixes > are probably not going to be contained to a single function > or file. Third, the flexible member array changes I made in > the past were not trivial, took time to develop, and the two > of us iterated over some of them for weeks. Despite your > careful review and my extensive testing some of them > introduced regressions that are still being patched up. > Fourth, making a change to reject code this close to a release > runs the risk of breaking code that has successfully compiled > in mass rebuilds and others' tests with the new compiler. > While that could be viewed as a good change for invalid code > that's exercised at run time, it could also break programs > where the bad code is never exercised. Fair enough. But I think the ICE is preferable to wrong code. Jason
> Since I've also spent some time on this: my take on this is that the C++ FE > should just follow C FE's suit and reject such initializations where > possible; it seems they've never worked reliably anyway, and bring more > harm than good. I don't see that rejecting such code would cause too much > trouble, if any, although the timing could be better. Schedule concerns aside, and although I agree that rejecting such code is far preferable to generating buggy programs, I'm not sure that rejecting auto initialization of flexible array members will ultimately lead to the best results. Users who want to initialize local objects with flexible array members will find some other (likely convoluted) way that GCC may not be able to find bugs in. A possible solution for those who can't or don't want to use std::array might be something like this: void f () { struct S { size_t n; int a[]; }; S *ps = (S*)alloca (sizeof (S) + 5); ps->n = 5; memcpy (ps->a, (const int[]){ 1, 2, 3, 4, 5 }, sizeof *ps->a * 5); ... } but that's clearly error-prone and hard to diagnose. GCC has no mechanism to detect bugs in this code without optimization, and because it folds the memcpy into a bunch of assignments, -Wstringop-overflow doesn't detect buffer overflows in the call to the function. I think it would be better to get the initialization to work so that users don't have to jump through hoops and can instead write cleaner code that more readily lends itself to checking for bugs. It might be even worth proposing local initialization as an enhancement for C2X. Martin
PR c++/79986 - [CHKP] ICE in fold_convert_loc with a flexible array gcc/ChangeLog: PR c++/79986 * tree-chkp.c (chkp_process_stmt): Avoid assuming size is non-null. gcc/testsuite/ChangeLog: PR c++/79986 * g++.dg/pr79986.C: New test. diff --git a/gcc/testsuite/g++.dg/pr79986.C b/gcc/testsuite/g++.dg/pr79986.C new file mode 100644 index 0000000..d179cf6 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr79986.C @@ -0,0 +1,10 @@ +/* PR c++/79986 - [CHKP] ICE in fold_convert_loc with a flexible array + { dg-do compile { target { { i?86-*-* x86_64-*-* } && { ! x32 } } } } + { dg-options "-fcheck-pointer-bounds -mmpx" } */ + +int f (int i) +{ + struct A { int n; char a[]; }; + + return ((struct A){ 1, "" }).a[i]; // { dg-error "invalid use of array" } +} diff --git a/gcc/tree-chkp.c b/gcc/tree-chkp.c index b1ff218..780d18f 100644 --- a/gcc/tree-chkp.c +++ b/gcc/tree-chkp.c @@ -4092,6 +4092,10 @@ chkp_process_stmt (gimple_stmt_iterator *iter, tree node, expression to compute it. */ if (!addr_last) { + /* C++ flexible array members have a null size. */ + if (!size) + return; + addr_last = fold_build_pointer_plus_loc (loc, addr_first, size); addr_last = fold_build_pointer_plus_hwi_loc (loc, addr_last, -1); }
Attached is a minimal patch to avoid an ICE in CHKP upon encountering one form of an initializer for a flexible array member, specifically the empty string: int f () { struct B { int n; char a[]; }; return ((struct B){ 1, "" }).a[0]; } Although GCC accepts (and doesn't ICE on) non-empty initializers for flexible array members, such as (struct B){ 1, "123" } it generates wrong code for them. This could either be fixed by emitting correct code, or it could be handled by rejecting all initializers for non-static objects with such members. Both approaches seem risky to me at this stage and so I think it's safest to hold off on implementing either until after the release. Martin