On Tue, Jun 15, 2021 at 9:57 AM Trevor Saunders <tbsau...@tbsaunde.org> 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 <tbsau...@tbsaunde.org>
> > 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 <tbsau...@tbsaunde.org>
> > > > 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<T, N> to auto_vec<T, N> is generated and just coppies the
> representation, its basically memcpy, so that doesn't work properly. If
> you attempt to copy auto_vec<T, P> to auto_vec<T, Q> 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<T> src)
> > > > {
> > > > auto_vec<T> dest (src);
> > > > ...
> > > > }
> > > >
> > > > bar()
> > > > {
> > > > auto_vec<X> a; // vs. auto_vec<X, 1>
> > > > a.safe_push (X()); // "decays" both to vec<X>
> > > > 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<x> &&src)
> > > {
> > > auto_vec<x> dst(src);
> > > ...
> > > }
> > >
> > > And then some caller who wants to use inline storage
> > > void bar()
> > > {
> > > auto-vec<x> 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<x> 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<T> 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<T> & 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<T>, so the compiler does not
> > > > > generate
> > > > > them for us. Per
> > > > > https://en.cppreference.com/w/cpp/language/move_constructor
> > > > > the ones taking vec<T> 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 <tbsau...@tbsaunde.org>
> > > > >
> > > > > bootstrapped and regtested on x86_64-linux-gnu, ok?
> > > > >
> > > > > gcc/ChangeLog:
> > > > >
> > > > > * vec.h (vl_ptr>::using_auto_storage): Handle null m_vec.
> > > > > (auto_vec<T, 0>::auto_vec): Define move constructor, and
> > > > > delete copy
> > > > > constructor.
> > > > > (auto_vec<T, 0>::operator=): Define move assignment and
> > > > > delete copy
> > > > > assignment.
> > > > > (auto_vec<T, N>::auto_vec): Delete copy and move constructors.
> > > > > (auto_vec<T, N>::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<T, va_heap, vl_embed> 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<T> &&r)
> > > > > + {
> > > > > + gcc_assert (!r.using_auto_storage ());
> > > > > + this->m_vec = r.m_vec;
> > > > > + r.m_vec = NULL;
> > > > > + }
> > > > > +
> > > > > auto_vec& operator= (vec<T, va_heap>&& 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<T> &&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<typename T>
> > > > > inline bool
> > > > > vec<T, va_heap, vl_ptr>::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
> > > > >
