have chkp skip flexible member arrays (PR #79986)

Submitted by Martin Sebor on March 20, 2017, 11:04 p.m.

Details

Message ID 4c6d8cdd-46f8-216d-56cb-07a74284f69a@gmail.com
State New
Headers show

Commit Message

Martin Sebor March 20, 2017, 11:04 p.m.
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

Comments

Jason Merrill March 20, 2017, 11:51 p.m.
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
Martin Sebor March 20, 2017, 11:58 p.m.
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
Jason Merrill March 21, 2017, 4:27 a.m.
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
Martin Sebor March 21, 2017, 3:08 p.m.
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
Jakub Jelinek March 21, 2017, 3:15 p.m.
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
Jeff Law March 21, 2017, 3:17 p.m.
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
Marek Polacek March 21, 2017, 3:46 p.m.
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
Jason Merrill March 21, 2017, 7:33 p.m.
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
Martin Sebor March 21, 2017, 11:56 p.m.
> 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

Patch hide | download patch | download mbox

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);
     }