From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x335.google.com (mail-ot1-x335.google.com [IPv6:2607:f8b0:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id E61E93855004 for ; Mon, 28 Jun 2021 18:07:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E61E93855004 Received: by mail-ot1-x335.google.com with SMTP id v5-20020a0568301bc5b029045c06b14f83so19686541ota.13 for ; Mon, 28 Jun 2021 11:07:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=CNtvYVBKK9tnh+FzI6o8r9khOFSBn9b73b5wlPZ9F5U=; b=mZTzQfDyQThFkTghiRlccmnGbHotIoRyM0JHR2/KxoQjK5FaG4XFmea2FYkWHipCWK Hu2yasemDM+t9asAim4G+QTFM4PWj05iFE6E8RlaFz6e6MLY7tdwITv5UoLND2Wsdwyx TkfKjAnkHTij4MleTXhfPefFZmQ37FpxtI2eRELHUajlHHhMX9DzsktBY6IKD1QDQgJc s1ZDN80ErHYEHBNvbCr87FLt2wZ6csKbZxPKm57S+ABGjDBq8oHV5RqEm2Gh/067NyiU 2+QlMyn7uRHSCFkPLbN981Vswb5lfUCQn6+Nzg9WbqKfvWbxvGPnfMDJ54BMLnEew/3l UkHw== X-Gm-Message-State: AOAM531SFncQ0CtxVKI56pVrM7lA1MHyg2Ec2A7KyXQzwj6100zkuyeP Jsc9Pg4TkdVjOQou4lcXzfU= X-Google-Smtp-Source: ABdhPJy1/C5CG57IqjOktYZhIs9im1TgXnLvwDApmHuyqrF6uXKxcqMX2EGXeG4sF+zEhubVHIPPFw== X-Received: by 2002:a9d:550e:: with SMTP id l14mr711032oth.241.1624903677344; Mon, 28 Jun 2021 11:07:57 -0700 (PDT) Received: from [192.168.0.41] (97-118-105-195.hlrn.qwest.net. [97.118.105.195]) by smtp.gmail.com with ESMTPSA id c21sm2201911otp.8.2021.06.28.11.07.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 28 Jun 2021 11:07:57 -0700 (PDT) Subject: Re: [PATCH] define auto_vec copy ctor and assignment (PR 90904) To: Richard Biener Cc: Jason Merrill , gcc-patches , Jonathan Wakely References: <91545a73-12af-33b2-c6e7-119b5a21de60@gmail.com> <4d503394-4e82-1d36-41ca-34315042775b@redhat.com> <49569f1d-9856-55c7-b9e9-578bbd7c7b7a@gmail.com> From: Martin Sebor Message-ID: Date: Mon, 28 Jun 2021 12:07:56 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jun 2021 18:07:59 -0000 On 6/28/21 2:07 AM, Richard Biener wrote: > On Sat, Jun 26, 2021 at 12:36 AM Martin Sebor wrote: >> >> On 6/25/21 4:11 PM, Jason Merrill wrote: >>> On 6/25/21 4:51 PM, Martin Sebor wrote: >>>> On 6/1/21 3:38 PM, Jason Merrill wrote: >>>>> On 6/1/21 3:56 PM, Martin Sebor wrote: >>>>>> On 5/27/21 2:53 PM, Jason Merrill wrote: >>>>>>> On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote: >>>>>>>> On 4/27/21 8:04 AM, Richard Biener wrote: >>>>>>>>> On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> On 4/27/21 1:58 AM, Richard Biener wrote: >>>>>>>>>>> On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> PR 90904 notes that auto_vec is unsafe to copy and assign because >>>>>>>>>>>> the class manages its own memory but doesn't define (or delete) >>>>>>>>>>>> either special function. Since I first ran into the problem, >>>>>>>>>>>> auto_vec has grown a move ctor and move assignment from >>>>>>>>>>>> a dynamically-allocated vec but still no copy ctor or copy >>>>>>>>>>>> assignment operator. >>>>>>>>>>>> >>>>>>>>>>>> The attached patch adds the two special functions to auto_vec >>>>>>>>>>>> along >>>>>>>>>>>> with a few simple tests. It makes auto_vec safe to use in >>>>>>>>>>>> containers >>>>>>>>>>>> that expect copyable and assignable element types and passes >>>>>>>>>>>> bootstrap >>>>>>>>>>>> and regression testing on x86_64-linux. >>>>>>>>>>> >>>>>>>>>>> The question is whether we want such uses to appear since those >>>>>>>>>>> can be quite inefficient? Thus the option is to delete those >>>>>>>>>>> operators? >>>>>>>>>> >>>>>>>>>> I would strongly prefer the generic vector class to have the >>>>>>>>>> properties >>>>>>>>>> expected of any other generic container: copyable and >>>>>>>>>> assignable. If >>>>>>>>>> we also want another vector type with this restriction I suggest >>>>>>>>>> to add >>>>>>>>>> another "noncopyable" type and make that property explicit in >>>>>>>>>> its name. >>>>>>>>>> I can submit one in a followup patch if you think we need one. >>>>>>>>> >>>>>>>>> I'm not sure (and not strictly against the copy and assign). >>>>>>>>> Looking around >>>>>>>>> I see that vec<> does not do deep copying. Making auto_vec<> do it >>>>>>>>> might be surprising (I added the move capability to match how vec<> >>>>>>>>> is used - as "reference" to a vector) >>>>>>>> >>>>>>>> The vec base classes are special: they have no ctors at all (because >>>>>>>> of their use in unions). That's something we might have to live with >>>>>>>> but it's not a model to follow in ordinary containers. >>>>>>> >>>>>>> I don't think we have to live with it anymore, now that we're >>>>>>> writing C++11. >>>>>>> >>>>>>>> The auto_vec class was introduced to fill the need for a conventional >>>>>>>> sequence container with a ctor and dtor. The missing copy ctor and >>>>>>>> assignment operators were an oversight, not a deliberate feature. >>>>>>>> This change fixes that oversight. >>>>>>>> >>>>>>>> The revised patch also adds a copy ctor/assignment to the auto_vec >>>>>>>> primary template (that's also missing it). In addition, it adds >>>>>>>> a new class called auto_vec_ncopy that disables copying and >>>>>>>> assignment as you prefer. >>>>>>> >>>>>>> Hmm, adding another class doesn't really help with the confusion >>>>>>> richi mentions. And many uses of auto_vec will pass them as vec, >>>>>>> which will still do a shallow copy. I think it's probably better >>>>>>> to disable the copy special members for auto_vec until we fix vec<>. >>>>>> >>>>>> There are at least a couple of problems that get in the way of fixing >>>>>> all of vec to act like a well-behaved C++ container: >>>>>> >>>>>> 1) The embedded vec has a trailing "flexible" array member with its >>>>>> instances having different size. They're initialized by memset and >>>>>> copied by memcpy. The class can't have copy ctors or assignments >>>>>> but it should disable/delete them instead. >>>>>> >>>>>> 2) The heap-based vec is used throughout GCC with the assumption of >>>>>> shallow copy semantics (not just as function arguments but also as >>>>>> members of other such POD classes). This can be changed by providing >>>>>> copy and move ctors and assignment operators for it, and also for >>>>>> some of the classes in which it's a member and that are used with >>>>>> the same assumption. >>>>>> >>>>>> 3) The heap-based vec::block_remove() assumes its elements are PODs. >>>>>> That breaks in VEC_ORDERED_REMOVE_IF (used in gcc/dwarf2cfi.c:2862 >>>>>> and tree-vect-patterns.c). >>>>>> >>>>>> I took a stab at both and while (1) is easy, (2) is shaping up to >>>>>> be a big and tricky project. Tricky because it involves using >>>>>> std::move in places where what's moved is subsequently still used. >>>>>> I can keep plugging away at it but it won't change the fact that >>>>>> the embedded and heap-based vecs have different requirements. >>>>>> >>>>>> It doesn't seem to me that having a safely copyable auto_vec needs >>>>>> to be put on hold until the rats nest above is untangled. It won't >>>>>> make anything worse than it is. (I have a project that depends on >>>>>> a sane auto_vec working). >>>>>> >>>>>> A couple of alternatives to solving this are to use std::vector or >>>>>> write an equivalent vector class just for GCC. >>>>> >>>>> It occurs to me that another way to work around the issue of passing >>>>> an auto_vec by value as a vec, and thus doing a shallow copy, would >>>>> be to add a vec ctor taking an auto_vec, and delete that. This would >>>>> mean if you want to pass an auto_vec to a vec interface, it needs to >>>>> be by reference. We might as well do the same for operator=, though >>>>> that isn't as important. >>>> >>>> Thanks, that sounds like a good idea. Attached is an implementation >>>> of this change. Since the auto_vec copy ctor and assignment have >>>> been deleted by someone else in the interim, this patch doesn't >>>> reverse that. I will propose it separately after these changes >>>> are finalized. >>>> >>>> My approach was to 1) disable the auto_vec to vec conversion, >>>> 2) introduce an auto_vec::to_vec() to make the conversion possible >>>> explicitly, and 3) resolve compilation errors by either changing >>>> APIs to take a vec by reference or callers to convert auto_vec to >>>> vec explicitly by to_vec(). In (3) I tried to minimize churn while >>>> improving the const-correctness of the APIs. >>> >>> What did you base the choice between reference or to_vec on? For >>> instance, it seems like c_parser_declaration_or_fndef could use a >>> reference, but you changed the callers instead. >> >> I went with a reference whenever I could. That doesn't work when >> there are callers that pass in a vNULL, so there, and in assignments, >> I used to_vec(). > > Is there a way to "fix" the ugliness with vNULL? All those functions > should be able to use const vec<>& as otherwise they'd leak memory? > Can't we pass vNULL to a const vec<>&? vNULL can bind to a const vec& (via the vec conversion ctor) but not to vec&. The three functions that in the patch are passed vNULL modify the argument when it's not vNULL but not otherwise. An alternate design is to have them take a vec* and pass in a plain NULL (or nullptr) instead of vNULL. That would require some surgery on the function bodies that I've been trying to avoid in the first pass. Functions that don't leak memory now shouldn't leak with these changes, and conversely, those that do will still leak. The patch doesn't change that (as far as I know). Going forward I think it's possible to replace most uses of vNULL in GCC with direct initialization (e.g., vec v{ }). Those that can't be readily replaced are the ones where vNULL is passed as an argument to functions taking a vec by value. Those could be changed to avoid vNULL too, but it would take a different approach and more effort. I'm not against it but I'd rather decouple those changes from this already sizeable patch. Martin > > Richard. > >> >> Martin >>