On 12/21/18 2:03 AM, Martin Sebor wrote:
> 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).
> 

Oh well, unfortunately the modified test case with static objects
fails one assertion when executed at -O0, I missed that before,
because I used -O2 or higher.  I filed that as PR 88578, so in the
moment I would like to leave the test case as compile only,
and change that to run once PR 88578 is resolved.

> 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.
> 

That is the case, because the array-6.c test case was moved
to c-c++-common.  That is the reproducer for the ICE from the PR.

> 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.
> 

Done.

I also added PR c++/69696 to the changelog as this should definitely
be fixed by this patch as well.


So despite the newly discovered problem with the non-constant
initializers which appears to be a separate problem, I would still like
to get an OK for this patch in the current form.


Thanks
Bernd.