From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) by sourceware.org (Postfix) with ESMTPS id A7176398A403 for ; Tue, 15 Jun 2021 09:36:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A7176398A403 Received: by mail-ej1-x633.google.com with SMTP id h24so21474432ejy.2 for ; Tue, 15 Jun 2021 02:36:27 -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=JE1i14Ssnm+zKop4YnrDRqCq4Tc4WqIR20IkYsTF0IM=; b=W/e734w1nicUeN+QqsCH+ShFtWTG6/bwsIIdptXYTRm4DcHCkFV6ys1cCi+bgCgkAm mBZTCYd45FN4Q7ZIaHZABRlsaZKJ0OtY3tj8HGN2MUQ4px486t9JprcBoykEA78HoxB2 L42Uhm8HifSaoTtd/5YhNTjaR79DDwTKqn9rv0kjUnIvcYRSbs7iVvE9NjDV5l/d6pwB ghcEDFVoYNYSGn5Sh6p7G2eRRWvTtMa0kLZmekByKJnmjZv0Q6JmWAyObfWgcKjmqeR1 oo1PYY2yLsPK1qDaPr3riqOSzxd2/JEA8rgCaaQoD5YR6SVnJe3VUG90ILQR033NIAs8 8+Yw== X-Gm-Message-State: AOAM532vLv4u/f3m1F/0gQ9H/yPkBXF1YYX0lFSZRCWvFZgA4dVPX5NV cewKYDbMJnQ3NE09ZcGP50N2EoEmyAC1+ilWUSdx2EcBMOw= X-Google-Smtp-Source: ABdhPJw8nL7cHndk5rNm09p4GX7Li0EkKMqQ2qW5RCdVN7ignbnjxcZy+VK6aHwvJqgsqzvR9mibB2RXALZP3GAp8eg= X-Received: by 2002:a17:907:2709:: with SMTP id w9mr19795381ejk.118.1623749786601; Tue, 15 Jun 2021 02:36:26 -0700 (PDT) MIME-Version: 1.0 References: <20210615055922.27205-1-tbsaunde@tbsaunde.org> In-Reply-To: From: Richard Biener Date: Tue, 15 Jun 2021 11:36:15 +0200 Message-ID: Subject: Re: [PATCH 1/6] auto_vec copy/move improvements To: Trevor Saunders Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, 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: Tue, 15 Jun 2021 09:36:29 -0000 On Tue, Jun 15, 2021 at 9:57 AM Trevor Saunders wrote: > > On Tue, Jun 15, 2021 at 09:11:52AM +0200, Richard Biener wrote: > > On Tue, Jun 15, 2021 at 9:04 AM Trevor Saunders wrote: > > > > > > On Tue, Jun 15, 2021 at 08:42:35AM +0200, Richard Biener wrote: > > > > On Tue, Jun 15, 2021 at 8:00 AM Trevor Saunders wrote: > > > > > > > > > > - Unfortunately using_auto_storage () needs to handle m_vec being null. > > > > > - Handle self move of an auto_vec to itself. > > > > > - punt on defining copy or move operators for auto_vec with inline storage, > > > > > until there is a need for them and we can decide what semantics they should > > > > > have. > > > > > > > > Hmm, that will make using of the CTORs/assignments in "infrastructure" > > > > fragile if you consider > > > > > > It definitely restricts what you can do with auto_vec with inline > > > storage. However that restriction is preexisting, and this just turns > > > it into a assertion failure rather than memory corruption. > > > > You mean the CTOR from vec<> is auto-generated at the moment? > > I actually had to test this to be sure. It depends, the constructor to > copy auto_vec to auto_vec is generated and just coppies the > representation, its basically memcpy, so that doesn't work properly. If > you attempt to copy auto_vec to auto_vec the compiler will > already refuse to generate the necessary constructor since the fields > don't match up, and so the case where the amount of inline storage is > different already fails to compile. > > > > So its > > > definitely not the final answer, but its better than what we have today > > > I believe, and leaves options open for when this has a user, as this > > > bootstraps nothing needs it today. > > > > > > > void foo(vec src) > > > > { > > > > auto_vec dest (src); > > > > ... > > > > } > > > > > > > > bar() > > > > { > > > > auto_vec a; // vs. auto_vec > > > > a.safe_push (X()); // "decays" both to vec > > > > foo (a); > > > > } > > > > > > > > that is, it will eventually lead to hard to track down results? I wonder if we > > > > should add a m_has_auto_storage and assert that the incoming vector > > > > does not instead of just asserting it doesn't use it to make the failure mode > > > > at least not dependent so much on "input"? > > > > > > I'm not sure I follow this part. I think example you are thinking of is > > > something like this > > > void foo(auto_vec &&src) > > > { > > > auto_vec dst(src); > > > ... > > > } > > > > > > And then some caller who wants to use inline storage > > > void bar() > > > { > > > auto-vec a; > > > a.safe_push (x ()); > > > foo (a); > > > } > > > > > > Which today I believe ends up with dst containing a pointer to part of > > > a, which is bogus and probably going to lead to memory corruption. > > > After this patch we get an assert when we try and create dst because > > > src.using_auto_storage () is true. That's certainly not ideal, but > > > better than what we have today. > > > > OK, so I guess one useful way to use the CTOR is when transfering vector > > ownership to a function, but I expected that > > > > void foo (auto_vec mine) > > { > > } > > > > would already do the trick here, destroying 'mine' when foo exits? > > Yes, after this patch you will have to move the vector into the argument > of the function with something like foo (move (freeby));. Today that > doesn't work because you get an implicitly generated copy constructor > for auto_vec that just coppies the pointer producing an auto_vec in > the callie and the caller that both think they own the same vector. > > > > > FWIW I agree that we likely want to avoid the copy that would be required > > > > when auto-storage is used - OTOH if we can be sure the lifetime of the > > > > result cannot be extended beyond the auto-storage provider then copying > > > > m_vec will likely just work? > > > > > > If I understand the case your thinking of correctly my question would be > > > why are you making a copy at all then, rather than passing a pointer or > > > reference to the original vector? I would think the two cases where a > > > copy may make sense is when the new object outlives the source, or when > > > you wish to mutate the new object leaving the original one unchanged, > > > for either of those copying the m_vec pointer so it points into the > > > original object wouldn't work? > > > > vec<> is used as (const) "reference" in a lot of places, avoiding the > > extra indirection that happens when using const vec<> & since passing > > its sole pointer member is cheap. (maybe const vec<> should be passed > > in all those cases though) > > Certainly the C++ way of doing things is to pass const vec & and hope > the abstraction gets optimized out. Its unfortunate, but its also not > really clear how else you'd mark that your giving the called function a > possibly const borrowed view of the vector. I suppose you can have yet > another type that you use in arguments and removes the layer of > abstraction when the called function doesn't need to mutate the vector. > Certainly if the call can mutate the vector you need to, and already do, > pass a vec<>& or vec<>*, and its just the case of functions that only > read the vector where this optimization can apply. I find myself > wondering how much this manual optimization matters, or if at this point > lto bootstrapping production compilers takes care of this for us? That's > a question I don't think I have the data to answer. > > > > > Besides this detail the patch looks OK. > > > > > > I think there's some risk of shooting yourself in the foot with the > > > inline storage version as it is today, but I'd be ok with spliting that > > > part out into a separate patch and only adjusting the version with no > > > inline storage here. I believe that's enough for the rest of the series > > > to work properly. > > > > I trust you with the change but I'm not too familiar with C++ to > > trust myself with a final OK, so if you can split out this part and > > post it separately that would make me more comfortable. > > Sure, just to be clear you mean the deleted constructors and assignment > operators for the version of auto_vec<> with inline storage? Yes. > I'll split > that out and make sure the series still bootstrapps and tests ok without > it, I really think it should but might as well confirm. Thanks, Richard. > Trev > > > > > Thanks, > > Richard. > > > > > > > > Thanks! > > > > > > Trev > > > > > > > > > > > Thanks, > > > > Richard. > > > > > > > > > - Make sure auto_vec defines the classes move constructor and assignment > > > > > operator, as well as ones taking vec, so the compiler does not generate > > > > > them for us. Per https://en.cppreference.com/w/cpp/language/move_constructor > > > > > the ones taking vec do not count as the classes move constructor or > > > > > assignment operator, but we want them as well to assign a plain vec to a > > > > > auto_vec. > > > > > - Explicitly delete auto_vec's copy constructor and assignment operator. This > > > > > prevents unintentional expenssive coppies of the vector and makes it clear > > > > > when coppies are needed that that is what is intended. When it is necessary to > > > > > copy a vector copy () can be used. > > > > > > > > > > Signed-off-by: Trevor Saunders > > > > > > > > > > bootstrapped and regtested on x86_64-linux-gnu, ok? > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > * vec.h (vl_ptr>::using_auto_storage): Handle null m_vec. > > > > > (auto_vec::auto_vec): Define move constructor, and delete copy > > > > > constructor. > > > > > (auto_vec::operator=): Define move assignment and delete copy > > > > > assignment. > > > > > (auto_vec::auto_vec): Delete copy and move constructors. > > > > > (auto_vec::operator=): Delete copy and move assignment. > > > > > --- > > > > > gcc/vec.h | 41 ++++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 40 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/gcc/vec.h b/gcc/vec.h > > > > > index 193377cb69c..ceefa67e1ad 100644 > > > > > --- a/gcc/vec.h > > > > > +++ b/gcc/vec.h > > > > > @@ -1549,6 +1549,16 @@ public: > > > > > this->release (); > > > > > } > > > > > > > > > > + // Punt for now on moving auto_vec with inline storage. For now this > > > > > + // prevents people creating dangling pointers or the like. > > > > > + auto_vec (auto_vec &&) = delete; > > > > > + auto_vec &operator= (auto_vec &&) = delete; > > > > > + > > > > > + // Punt for now on the inline storage, and you probably don't want to copy > > > > > + // vectors anyway. If you really must copy a vector use copy (). > > > > > + auto_vec(const auto_vec &) = delete; > > > > > + auto_vec &operator= (const auto_vec &) = delete; > > > > > + > > > > > private: > > > > > vec m_auto; > > > > > T m_data[MAX (N - 1, 1)]; > > > > > @@ -1570,14 +1580,43 @@ public: > > > > > this->m_vec = r.m_vec; > > > > > r.m_vec = NULL; > > > > > } > > > > > + > > > > > + auto_vec (auto_vec &&r) > > > > > + { > > > > > + gcc_assert (!r.using_auto_storage ()); > > > > > + this->m_vec = r.m_vec; > > > > > + r.m_vec = NULL; > > > > > + } > > > > > + > > > > > auto_vec& operator= (vec&& r) > > > > > { > > > > > + if (this == &r) > > > > > + return *this; > > > > > + > > > > > + gcc_assert (!r.using_auto_storage ()); > > > > > + this->release (); > > > > > + this->m_vec = r.m_vec; > > > > > + r.m_vec = NULL; > > > > > + return *this; > > > > > + } > > > > > + > > > > > + auto_vec& operator= (auto_vec &&r) > > > > > + { > > > > > + if (this == &r) > > > > > + return *this; > > > > > + > > > > > gcc_assert (!r.using_auto_storage ()); > > > > > this->release (); > > > > > this->m_vec = r.m_vec; > > > > > r.m_vec = NULL; > > > > > return *this; > > > > > } > > > > > + > > > > > + // You probably don't want to copy a vector, so these are deleted to prevent > > > > > + // unintentional use. If you really need a copy of the vectors contents you > > > > > + // can use copy (). > > > > > + auto_vec(const auto_vec &) = delete; > > > > > + auto_vec &operator= (const auto_vec &) = delete; > > > > > }; > > > > > > > > > > > > > > > @@ -2147,7 +2186,7 @@ template > > > > > inline bool > > > > > vec::using_auto_storage () const > > > > > { > > > > > - return m_vec->m_vecpfx.m_using_auto_storage; > > > > > + return m_vec ? m_vec->m_vecpfx.m_using_auto_storage : false; > > > > > } > > > > > > > > > > /* Release VEC and call release of all element vectors. */ > > > > > -- > > > > > 2.20.1 > > > > >