Re: [PATCH 1/6] auto_vec copy/move improvements

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Jun 15, 2021 at 9:04 AM Trevor Saunders <tbsaunde@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 <tbsaunde@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 <tbsaunde@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
> > >