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

[Martin Sebor <msebor@gmail.com>]

On 6/2/21 12:55 AM, Richard Biener wrote:
> On Tue, Jun 1, 2021 at 9:56 PM Martin Sebor <msebor@gmail.com> 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.
> 
> So you figured that neither vec<> nor auto_vec<> are a container like
> std::vector.

That's obvious from glancing at their definitions.  I didn't go
through the exercise to figure that out.

> 
> I'm not sure it makes sense to try to make it so since obviously vec<>
> was designed to match the actual needs of GCC.  auto_vec<> was added
> to make a RAII (like auto_bitmap, etc.) wrapper, plus it got the ability
> to provide initial stack storage.

The goal was to see if the two vec instances could be made safer
to use but taking advantage of C++ 11 features.  As I mentioned
recently, creating a copy of a vec and modifying it changes it as
well as the original (e.g., by changing a vec argument passed to
it by value a function changes the actual argument in the caller).
That's surprising to most C++ programmers.

My conclusion from the exercise is that although some of the problems
with vec can, and IMO should, be solved, making the heap-based one
a well-behaved C++ 11 container will take considerable effort and
is impossible for the embedded vec.

> 
>> 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).
> 
> So how does your usage look like?  I can't really figure who'd need
> deep copying of a container - note there's vec<>::copy at your
> discretion.
> 
>> A couple of alternatives to solving this are to use std::vector or
>> write an equivalent vector class just for GCC.
> 
> As said, can you show the usage that's impossible to do with
> the current vec<>/auto_vec<>?

The test case in PR 90904 shows a trivial example.  More generally,
using an auto_vec that depends on it being assignable (e.g., storing
an auto_vec in another container like hash_map or auto_vec itself)
is impossible.  Using a plain vec requires manual memory management
and so is error-prone.

But more important, as a C++ code base, GCC should follow the best
practices for the language.  Among the essential ones are using RAII
to manage resources and the Rule of Three (or Five in C++ 11): a class
that defines a dtor should also define a copy ctor and copy assignment
(and move ctor and move assignment in C++).

Martin