[PATCH] define auto_vec copy ctor and assignment (PR 90904)

Trevor Saunders tbsaunde@tbsaunde.org
Tue Jun 29 12:30:55 GMT 2021


On Fri, Jun 25, 2021 at 02:51:58PM -0600, Martin Sebor via Gcc-patches wrote:
> On 6/1/21 3:38 PM, Jason Merrill wrote:
> > On 6/1/21 3:56 PM, Martin Sebor wrote:
> > > On 5/27/21 2:53 PM, Jason Merrill wrote:
> > > > On 4/27/21 11:52 AM, Martin Sebor via Gcc-patches wrote:
> > > > > On 4/27/21 8:04 AM, Richard Biener wrote:
> > > > > > On Tue, Apr 27, 2021 at 3:59 PM Martin Sebor <msebor@gmail.com> wrote:
> > > > > > > 
> > > > > > > On 4/27/21 1:58 AM, Richard Biener wrote:
> > > > > > > > On Tue, Apr 27, 2021 at 2:46 AM Martin Sebor via Gcc-patches
> > > > > > > > <gcc-patches@gcc.gnu.org> wrote:
> > > > > > > > > 
> > > > > > > > > PR 90904 notes that auto_vec is unsafe to copy and assign because
> > > > > > > > > the class manages its own memory but doesn't define (or delete)
> > > > > > > > > either special function.  Since I first ran into the problem,
> > > > > > > > > auto_vec has grown a move ctor and move assignment from
> > > > > > > > > a dynamically-allocated vec but still no copy ctor or copy
> > > > > > > > > assignment operator.
> > > > > > > > > 
> > > > > > > > > The attached patch adds the two special functions to auto_vec along
> > > > > > > > > with a few simple tests.  It makes auto_vec
> > > > > > > > > safe to use in containers
> > > > > > > > > that expect copyable and assignable element
> > > > > > > > > types and passes bootstrap
> > > > > > > > > and regression testing on x86_64-linux.
> > > > > > > > 
> > > > > > > > The question is whether we want such uses to appear since those
> > > > > > > > can be quite inefficient?  Thus the option is to
> > > > > > > > delete those operators?
> > > > > > > 
> > > > > > > I would strongly prefer the generic vector class to
> > > > > > > have the properties
> > > > > > > expected of any other generic container: copyable and assignable.  If
> > > > > > > we also want another vector type with this
> > > > > > > restriction I suggest to add
> > > > > > > another "noncopyable" type and make that property
> > > > > > > explicit in its name.
> > > > > > > I can submit one in a followup patch if you think we need one.
> > > > > > 
> > > > > > I'm not sure (and not strictly against the copy and
> > > > > > assign). Looking around
> > > > > > I see that vec<> does not do deep copying.  Making auto_vec<> do it
> > > > > > might be surprising (I added the move capability to match how vec<>
> > > > > > is used - as "reference" to a vector)
> > > > > 
> > > > > The vec base classes are special: they have no ctors at all (because
> > > > > of their use in unions).  That's something we might have to live with
> > > > > but it's not a model to follow in ordinary containers.
> > > > 
> > > > I don't think we have to live with it anymore, now that we're
> > > > writing C++11.
> > > > 
> > > > > The auto_vec class was introduced to fill the need for a conventional
> > > > > sequence container with a ctor and dtor.  The missing copy ctor and
> > > > > assignment operators were an oversight, not a deliberate feature.
> > > > > This change fixes that oversight.
> > > > > 
> > > > > The revised patch also adds a copy ctor/assignment to the auto_vec
> > > > > primary template (that's also missing it).  In addition, it adds
> > > > > a new class called auto_vec_ncopy that disables copying and
> > > > > assignment as you prefer.
> > > > 
> > > > Hmm, adding another class doesn't really help with the confusion
> > > > richi mentions.  And many uses of auto_vec will pass them as
> > > > vec, which will still do a shallow copy.  I think it's probably
> > > > better to disable the copy special members for auto_vec until we
> > > > fix vec<>.
> > > 
> > > There are at least a couple of problems that get in the way of fixing
> > > all of vec to act like a well-behaved C++ container:
> > > 
> > > 1) The embedded vec has a trailing "flexible" array member with its
> > > instances having different size.  They're initialized by memset and
> > > copied by memcpy.  The class can't have copy ctors or assignments
> > > but it should disable/delete them instead.
> > > 
> > > 2) The heap-based vec is used throughout GCC with the assumption of
> > > shallow copy semantics (not just as function arguments but also as
> > > members of other such POD classes).  This can be changed by providing
> > > copy and move ctors and assignment operators for it, and also for
> > > some of the classes in which it's a member and that are used with
> > > the same assumption.
> > > 
> > > 3) The heap-based vec::block_remove() assumes its elements are PODs.
> > > That breaks in VEC_ORDERED_REMOVE_IF (used in gcc/dwarf2cfi.c:2862
> > > and tree-vect-patterns.c).
> > > 
> > > I took a stab at both and while (1) is easy, (2) is shaping up to
> > > be a big and tricky project.  Tricky because it involves using
> > > std::move in places where what's moved is subsequently still used.
> > > I can keep plugging away at it but it won't change the fact that
> > > the embedded and heap-based vecs have different requirements.
> > > 
> > > It doesn't seem to me that having a safely copyable auto_vec needs
> > > to be put on hold until the rats nest above is untangled.  It won't
> > > make anything worse than it is.  (I have a project that depends on
> > > a sane auto_vec working).
> > > 
> > > A couple of alternatives to solving this are to use std::vector or
> > > write an equivalent vector class just for GCC.
> > 
> > It occurs to me that another way to work around the issue of passing an
> > auto_vec by value as a vec, and thus doing a shallow copy, would be to
> > add a vec ctor taking an auto_vec, and delete that.  This would mean if
> > you want to pass an auto_vec to a vec interface, it needs to be by
> > reference.  We might as well do the same for operator=, though that
> > isn't as important.
> 
> Thanks, that sounds like a good idea.  Attached is an implementation
> of this change.  Since the auto_vec copy ctor and assignment have
> been deleted by someone else in the interim, this patch doesn't
> reverse that.  I will propose it separately after these changes
> are finalized.
> 
> My approach was to 1) disable the auto_vec to vec conversion,
> 2) introduce an auto_vec::to_vec() to make the conversion possible
> explicitly, and 3) resolve compilation errors by either changing
> APIs to take a vec by reference or callers to convert auto_vec to
> vec explicitly by to_vec().  In (3) I tried to minimize churn while
> improving the const-correctness of the APIs.

In terms of naming, while I agree with Richi this would ideally just go
away, in the mean time as_vec seems like a clearer name to me, since its
more of an unsafe cast than a transfer of ownership to the return value.

The patch is very large, and while I tend to think its probably
reasonable, and it is largly mechanical, it would still be easier to
deal with if it was broken up, by the API being changed.  Changing some
of the functions to use array_slice would probably be better than const
vec &, and eliminate the concern about indirection by actually removing
one indirection which seems modestly worthwhile.

One other concern would be its still possible to do something like
auto_vec<int> x;
const vec<int> &y = x;
vec<int> z = y;

So it might be better to disable the conversion with a operator vec<> ()
on auto_vec that is deleted?

> When changing a vec API from by-value to by-reference I used vec &
> and didn't try to use vec * even though the latter seems to be more
> common (I avoided this to minimize the scope of changes).  If we
> want to do this I suggest handling that in a separate change.
> 
> It's possible to get rid of vNULL in initialization.  It can be
> replaced with value-initialization in declarations but it's still
> handy when passing "empty" AKA nil vecs as arguments.  I didn't
> do it except in one instance (vec::copy) as a POC.

 In principal it makes sense to do this, as much as it would be nice for
 the non gc vec to just go away.  However it adds unnecessary changes to
 a large mostly mechanical patch, so I think it would be better broken
 out into a separate patch.  While I believe it should work it certainly
 wouldn't be the first time this sort of change found a compiler bug, so
 you should probably at least test it with gcc 4.8 to see it handles it,
 and this increases the value of separating it from other changes?

> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index c4eb2b1c920..9841a320f89 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -1115,8 +1115,8 @@ c_build_vec_perm_expr (location_t loc, tree v0, tree v1, tree mask,
>     and have vector types, V0 has the same element type as V1, and the
>     number of elements the result is that of MASK.  */
>  tree
> -c_build_shufflevector (location_t loc, tree v0, tree v1, vec<tree> mask,
> -		       bool complain)
> +c_build_shufflevector (location_t loc, tree v0, tree v1,
> +		       const vec<tree> &mask, bool complain)
>  {
>    tree ret;
>    bool wrap = true;
> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> index 88022d0b0a9..e43b12ae1dc 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -1049,7 +1049,7 @@ extern bool vector_targets_convertible_p (const_tree t1, const_tree t2);
>  extern bool vector_types_convertible_p (const_tree t1, const_tree t2, bool emit_lax_note);
>  extern tree c_build_vec_perm_expr (location_t, tree, tree, tree, bool = true);
>  extern tree c_build_shufflevector (location_t, tree, tree,
> -				   vec<tree>, bool = true);
> +				   const vec<tree> &, bool = true);
>  extern tree c_build_vec_convert (location_t, tree, location_t, tree, bool = true);
>  
>  extern void init_c_lex (void);
> diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
> index 27034f88f49..e4a85eeb817 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -1780,7 +1780,7 @@ c_parser_external_declaration (c_parser *parser)
>      }
>  }
>  
> -static void c_finish_omp_declare_simd (c_parser *, tree, tree, vec<c_token>);
> +static void c_finish_omp_declare_simd (c_parser *, tree, tree, vec<c_token> &);
>  static void c_finish_oacc_routine (struct oacc_routine_data *, tree, bool);
>  
>  /* Build and add a DEBUG_BEGIN_STMT statement with location LOC.  */
> @@ -20325,12 +20325,12 @@ c_parser_omp_declare_simd (c_parser *parser, enum pragma_context context)
>  	  while (c_parser_next_token_is (parser, CPP_KEYWORD)
>  		 && c_parser_peek_token (parser)->keyword == RID_EXTENSION);
>  	  c_parser_declaration_or_fndef (parser, true, true, true, false, true,
> -					 NULL, clauses);
> +					 NULL, clauses.to_vec ());

As an example of doing this piece by piece rather than as a mass change,
I suspect this could pass an array_slice, and I wonder if we could just
move the vec to the callee, c_finish_omp_declare_simd_clauses seems to
be the only user, and while a quick look doesn't answer the question, I
suspect from the name it should only be called once in
c_parser_declaration_or_fndecl, but that would take some investigation,
and I suppose can be done incrementally.

Trev



More information about the Gcc-patches mailing list