[PATCH 1/6] auto_vec copy/move improvements

Trevor Saunders tbsaunde@tbsaunde.org
Tue Jun 15 07:04:30 GMT 2021


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

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

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

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
> >


More information about the Gcc-patches mailing list