[PATCH, C++,rebased] Fix PR c++/88261

[Bernd Edlinger]

On 12/22/18 7:53 PM, Bernd Edlinger wrote:
> 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.


The patch itself is unchanged, the new version fixes a merge conflict due
to the recently added parameter stripped_init of process_init_constructor.


Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
Is it OK for trunk?

Thanks
Bernd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-pr88261.diff
Type: text/x-patch
Size: 20275 bytes
Desc: patch-pr88261.diff
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190104/887213dd/attachment.bin>