From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) by sourceware.org (Postfix) with ESMTPS id CCCED385380C for ; Mon, 7 Jun 2021 08:51:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CCCED385380C Received: by mail-ej1-x634.google.com with SMTP id a11so24660260ejf.3 for ; Mon, 07 Jun 2021 01:51:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2xjDTyli1rMmLvUEXQJKgE8ulWMlMPto7TIT3gg1iUM=; b=jLUC1BTT9oa116EyAMmyqdQtSULB0se3AXE5jRURKfjwYnhNkU5vIFCLyJH001dcCP DQrtKKWFWu2Vu46u9XykC0GQ6O5zHJoQArDY1Q7R9dsPR9+okACz7BBe3npc/EEXj0ol mRpfug+9CM6sbNrdHF3QoJWR/WoJxMeBRMEc647kLwzVlJTNMKb3rS6YMm+aacV537zV k7Q6uk4Jf3oddGJR2b76onabD12PgAZh97WeKvh+TbtvCYsk+I7/kn+RJtyLR+Pb4F1J Q6vQ1a6f5MZsW184sH8j489bQU3Yyy2cLJgZwVzeD78J1w9/VXOl788Tqm0KtaAiW8Lq Sb+Q== X-Gm-Message-State: AOAM530KyfBuw80fELv0X03g1hzteY16ZfMtP2sp9NskHaJrSFfpWuYg zqMZllwvhRqZPFs2IPueDayPw9lVhmDGJYOkk/8= X-Google-Smtp-Source: ABdhPJxzQC8Szudxf5xmIb43EIeNscPXwkm0ciNik1ttUwHz5tPc7x/oAoQG9AOL1A+Ye+j3VO7k4NNMnIhaaVtq85I= X-Received: by 2002:a17:906:e253:: with SMTP id gq19mr17264814ejb.138.1623055889800; Mon, 07 Jun 2021 01:51:29 -0700 (PDT) MIME-Version: 1.0 References: <91545a73-12af-33b2-c6e7-119b5a21de60@gmail.com> <4d503394-4e82-1d36-41ca-34315042775b@redhat.com> <372f08c6-9466-3bdb-a246-0815e1b84047@gmail.com> In-Reply-To: From: Richard Biener Date: Mon, 7 Jun 2021 10:51:18 +0200 Message-ID: Subject: Re: [PATCH] define auto_vec copy ctor and assignment (PR 90904) To: Trevor Saunders Cc: Martin Sebor , Jonathan Wakely , gcc-patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, 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, 07 Jun 2021 08:51:41 -0000 On Thu, Jun 3, 2021 at 10:29 AM Trevor Saunders wrote: > > On Wed, Jun 02, 2021 at 10:04:03AM -0600, Martin Sebor via Gcc-patches wrote: > > On 6/2/21 12:55 AM, Richard Biener wrote: > > > On Tue, Jun 1, 2021 at 9: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. > > I've been away a while, but trying to get back into this, sorry. It was > definitely an oversight to leave these undefined for the compiler to > provide a default definition of, but I agree with Richi, the better > thing to have done, or do now would be to mark them as deleted and make > auto_vec move only (with copy() for when you really need a deep copy. > > > > > > > > > > > > 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. > > > > > > So you figured that neither vec<> nor auto_vec<> are a container like > > > std::vector. > > > > That's obvious from glancing at their definitions. I didn't go > > through the exercise to figure that out. > > > > > > > > I'm not sure it makes sense to try to make it so since obviously vec<> > > > was designed to match the actual needs of GCC. auto_vec<> was added > > > to make a RAII (like auto_bitmap, etc.) wrapper, plus it got the ability > > > to provide initial stack storage. > > > > The goal was to see if the two vec instances could be made safer > > to use but taking advantage of C++ 11 features. As I mentioned > > recently, creating a copy of a vec and modifying it changes it as > > well as the original (e.g., by changing a vec argument passed to > > it by value a function changes the actual argument in the caller). > > That's surprising to most C++ programmers. > > It can probably be improved now with c++11, but while very unfortunate > There is hard requirements on how vec works from existing code using it. > > > My conclusion from the exercise is that although some of the problems > > with vec can, and IMO should, be solved, making the heap-based one > > a well-behaved C++ 11 container will take considerable effort and > > is impossible for the embedded vec. > > Yes, fortunately things using embedded vec do not at all expect a c++ > container, and so don't really mismatch it. You probably should not be > creating them yourself unless you are creating a new object with an > embedded vector, and you probably don't want to do that. > > > > > > > > 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). > > > > > > So how does your usage look like? I can't really figure who'd need > > > deep copying of a container - note there's vec<>::copy at your > > > discretion. > > > > > > > A couple of alternatives to solving this are to use std::vector or > > > > write an equivalent vector class just for GCC. > > imho one of the significant advantages to having our own datastructures > rather than using the standard library is the ability to have a > different API that is less constrained by history, and can make better > choices than standard containers like deleting operators that would > otherwise require deep coppies. Though certainly they don't always live > up to that like the oversight here of not defining the copy / assignment > operators at all. Perhaps there's an argument to be made for the > standard containers doing deep coppies that it makes the language easier > to use, but its not all that much easier than .copy(), if that's your > priority c++ probably isn't the right tool for the job, and I doubt it > makes sense for gcc in particular. > > > > As said, can you show the usage that's impossible to do with > > > the current vec<>/auto_vec<>? > > > > The test case in PR 90904 shows a trivial example. More generally, > > using an auto_vec that depends on it being assignable (e.g., storing > > an auto_vec in another container like hash_map or auto_vec itself) > > is impossible. Using a plain vec requires manual memory management > > and so is error-prone. Btw, I remember once trying to make hash_map > work which pre-dated C++11 allowance (but I found a much nicer, albeit non-"C++" solution using obstacks and linked lists .. heh). That might work nowadays if we fix hash_map re-allocation to use std::move and add move CTORs to the auto_vec template (I refrained from that when I added them to the , 0 specialization since even moving would mean moving quite some storage). > Certainly deleting the copy constructor and assignment operator means > that you can't use them, but can you show real code where it is a > significant imposition to have to call .copy() rather than using them? > Certainly its a little longer, but deep copies are a bit of a > performance footgun, especially when you have vectors linear in the size > of the function all over, and your goal is to be no worse than > O(N log(N)), meaning you can copy the vector at most log(N) times at > worst. > > I would think storing move only objects in auto_vec and hash_* should > work, and if it doesn't should be fixable without introducing overly > easy ways to make deep coppies. > > > But more important, as a C++ code base, GCC should follow the best > > practices for the language. Among the essential ones are using RAII > > to manage resources and the Rule of Three (or Five in C++ 11): a class > > that defines a dtor should also define a copy ctor and copy assignment > > (and move ctor and move assignment in C++). > > When discussing the rule of 3/5 at least > https://en.cppreference.com/w/cpp/language/rule_of_three considers > deleting the member to be a form of definition, see the part about non > copiable members and deleting both copy constructor and assignment, in > this case to make the class move only. Strictly speaking, I suppose its > true that an array of 10k items is copiable, but its also very likely > something to be avoided if at all possible, and doesn't need to be made > easy. > > Trev > > > > > Martin