Re: [PATCH, C++] Fix PR c++/88261

[Martin Sebor <msebor@gmail.com>]

On 12/20/18 2:07 PM, Bernd Edlinger wrote:
> On 12/20/18 6:50 PM, Martin Sebor wrote:
>> On 12/20/18 10:46 AM, Martin Sebor wrote:
>>> On 12/17/18 7:58 AM, Jason Merrill wrote:
>>>> On 12/15/18 3:36 AM, Bernd Edlinger wrote:
>>>>> this patch implements an error message, for non-static initialization of a flexible array member.
>>>>> This duplicates the existing error message from the C-FE, to avoid ICE and wrong code generation
>>>>> issues, as pointed out in the PR.
>>>>>
>>>>> It is a bit funny that a non-functional feature like that has already rather much test coverage.
>>>>> The most easy adjustment seems to change the existing test cases to use static declarations.
>>>>
>>>> Martin, thoughts?
>>>
>>> Our high-level goal when tightening up how flexible array members
>>> are handled in C++ was to accept what's accepted in standard C mode
>>> and reject (or, at a minimum, warn for) C++ extensions that could
>>> be relied on in existing code.
>>
>> I meant "reject what couldn't be relied on" and "warn for that could
>> be."
>>
> 
> I believe the problem here is effectively that initializing non-static
> flexible array is not supported by the middle-end.  All examples
> where flexible array members are initialized on automatic variable
> work only as long as they are simple enough that they are optimized
> away so that they do not survive until expansion.
> 
> Take as example gcc/testsuite/g++.dg/ext/flexary13.C,
> it compiles and runs successfully, but the assertions start to
> fail if Ax is declared volatile, and at the same time, we know
> that the automatic variables are allocated in a way that they
> can overlap and crash at any time.
> 
> My impression is that the existing C error made the middle-end kind of rely
> on this behavior.
> 
> So I think the right thing to do is duplicate the existing C error in
> the C++ FE.  I do not see any automatic variable with initialized flexible
> data members where it would be safe to only warn about them.

If there are no reasonable use cases that code out there could
be relying on because none of them works correctly then rejecting
the initialization makes sense to me.

>> (Sorry for the delay, by the way.  I've been migrating to a new machine
>> this week and things aren't yet working quite like I'm used to.)
>>
>>>
>>> The flexarray tests I added back then were for features that looked
>>> like intentional extensions and that seemed to work for at least
>>> some use cases as far as I could tell.  What I noticed didn't work
>>> I created bugs for: 69338, 69696, and 69338 look related, but there
>>> are others.
>>>
>>> I think all these bugs should all be reviewed and a decision made
>>> about what's intended to work and what continues to be accepted as
>>> an accident and should be rejected.  After that, we can adjust
>>> the existing tests.
>>>
> 
> I would not rule out the possibility that there can be more bugs.
> But I think the existing tests need to avoid the case which evokes
> the new error.  The question is, if changing from automatic to static
> objects prevents those tests to test what they were originally written for.
> I believe this is not the case, but I do probably not know all the
> background here.

IIRC, most of the tests I added were meant to exercise just
the front-end, not any later stages (if that's what you meant).
Otherwise, if you're worried about the changes from auto to
static no longer exercising downstream front-end code, whether
that matters depends on the intent of each test.

flexary13.C was most likely meant to also verify codegen (hence
the assertions) so I would suggest to make it do that (i.e.,
verify the assertions are optimized out if in fact they are,
or make the test run so they must pass).

The changes to the rest of the flexary*.C tests seem okay,
though a new test should be added to explicitly exercise this
change (bug 88261), even if the error happens to be tested by
one of the changed tests.

In changes to the Wplacement-new-size*.C tests I would suggest
to follow the same approach of using statics instead of testing
for errors so the code that exercises warnings doesn't depend
on erroneous constructs.

The comment in Wplacement-new-size-2.C just above the code your
patch changes that reads:

   // Initialization of non-static objects with flexible array members
   // isn't allowed in C and should perhaps be disallowed in C++ as
   // well to avoid c++/69696 - incorrect initialization of block-scope
   // flexible array members.
   Ax ax2 = { 1, { 2, 3 } };

should be updated and the referenced bug and any others that this
change prevents should be resolved.

Martin