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?
> 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?
> > 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)
> > 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.
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
> > >
