From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19782 invoked by alias); 4 Jan 2019 21:23:05 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 19759 invoked by uid 89); 4 Jan 2019 21:23:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=BAYES_00,KAM_MANYTO,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=fe, accident, uncomfortable, Edlinger X-HELO: mail-qk1-f193.google.com Received: from mail-qk1-f193.google.com (HELO mail-qk1-f193.google.com) (209.85.222.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 04 Jan 2019 21:23:01 +0000 Received: by mail-qk1-f193.google.com with SMTP id o8so5890459qkk.11 for ; Fri, 04 Jan 2019 13:23:01 -0800 (PST) Return-Path: Received: from [192.168.1.149] (209-6-216-142.s141.c3-0.smr-cbr1.sbo-smr.ma.cable.rcncustomer.com. [209.6.216.142]) by smtp.gmail.com with ESMTPSA id o42sm33779035qtc.90.2019.01.04.13.22.58 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 04 Jan 2019 13:22:59 -0800 (PST) Subject: Re: [PATCH, C++,rebased] Fix PR c++/88261 To: Bernd Edlinger , Martin Sebor , Martin Sebor , "gcc-patches@gcc.gnu.org" , Jeff Law , Nathan Sidwell References: <4a92d8cf-a0da-67ac-d544-2c87f9d3a07e@redhat.com> <6f8f1380-1f13-007d-03e6-a2fe3d0d62f1@gmail.com> From: Jason Merrill Message-ID: <18e39add-b0de-e476-86ba-3422dc911b6a@redhat.com> Date: Fri, 04 Jan 2019 21:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-01/txt/msg00179.txt.bz2 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? Jason