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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 1/4/19 10:22 PM, Jason Merrill wrote:
> On 1/4/19 10:30 AM, Bernd Edlinger wrote:
>> 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.
> 
> Hmm, I'm uncomfortable with starting to pass in the decl just for the sake of deciding whether this diagnostic should be a pedwarn or error. In general, because of copy elision, we can't know at this point what we're initializing, so I'd rather not pretend we can.  Instead, maybe add a LOOKUP_ALLOW_FLEXARY_INIT flag that you can add to the flags argument in the call from store_init_value?
> 

Yes that sounds reasonable, just I think the error or warning is
always emitted from digest_nsdmi_init.
I have never seen that happening from store_init_value.

I'll give it a try and come back with an updated patch.


Bernd.