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.