RFR: 8213481: [REDO] Fix incorrect copy constructors in hotspot

[Kim Barrett]

> On Dec 3, 2018, at 2:33 AM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 3/12/2018 5:03 pm, Kim Barrett wrote:
>>> On Nov 27, 2018, at 4:34 PM, David Holmes <david.holmes at oracle.com> wrote:
>>> 
>>> On 28/11/2018 4:12 am, Kim Barrett wrote:
>>>> The "natural result" is that the derived class copy-assign code does
>>>> what it does.  There are things that can go badly wrong here regarding
>>>> lifetimes of embedded object references.  But the asserted restriction
>>>> on the allocated location of the object being copy-assigned to does
>>>> nothing that I can see to prevent those potential problems.
>>>> Can you provide a use-case where the assertion actually does something
>>>> useful?  I removed it because I don’t think such a thing exists.
>>> 
>>> AFAICS the only time it makes any sense to use assignment is with stack/embedded ResourceObj. The assert verifies that is the case.
>>> 
>>> This seems a very clear cut use of an assert to ensure an API is not being misused. Does it prevent all possible misuse - no. But that's not a reason to remove what it does do.
>>> 
>>> Do we ever need the assignment semantics? Maybe we should just prohibit it altogether?
>>> 
>>> David
>> I don't understand why you might think that, and don't agree.  Maybe
>> there are additional unstated and unenforced restrictions?
>> But part of my rejection of such a position is that a commonly
>> understood idiomatic operation like assignment should either just
>> work, or it shouldn't be allowed at all. Having a strange restriction
>> like this makes code much harder to understand. Make a new operation
>> for this restricted assignment.
> 
> How do you make a special assignment operator? AFAIK there is only one op=

You spell it differently and forbid op=, e.g. something like
  ResourceObj& assign_living_dangerously(const ResourceObj&);
  ResourceObj& operator=(const ResourceObj&) = delete; // C++11

> ResourceObj is not a nice clean, well-defined class

I can certainly agree with that!

> - it is one class that embodies the possibility of being one of three different kinds of "resource obj" (embedded/stack, arena-allocated, C-heap allocated) and the operations of copying and assignment don't make sense for all of these - hence the restriction.

A ResourceObj cannot have ownership of any contained allocated objects
(ownership in the sense of having responsibility for deallocation),
because the resource and arena mechanisms by design do not call the
destructor of relevant objects. (Such destructor avoidance is allowed
by C++ 3.8/4, so long as the destructor doesn't have any side effects
the application depends on.)

So copy and copy-assign for a class derived from ResourceObj (and so
possibly resource allocated) can't really do anything more than a
shallow copy unless it can somehow know that it was not resource
allocated. The relevant API provided by ResourceObj is DEBUG_ONLY, so
can't be used for conditionalization, only assertion.

Some classes (GrowableArray, for example), have their own mechanism
for determining allocation location and ResourceMark nesting when
relevant. GrowableArray is actually a pretty good example of how
things can go awry right now. It has the default copy constructor and
copy assignment operator, which are both wrong and better not be used
in some cases.

Both copy and copy-assign when the copy source is on_C_heap will
result in double-free.  (The ResourceObj copy-assign assert doesn't
help here.)

Copy-assign to an on_C_heap object will leak the old data array.
(The ResourceObj copy-assign assert doesn't help very much here
either.  The array can be stack/member allocated but configured to
allocate its data from the C heap; that's an entirely reasonable
configuration.)

Both of those could be addressed by providing the appropriate
definition (similar to what, for example, std::vector does).

Of course, resource/arena allocating a growable array configured to
use the C heap for its data is a mistake, since the destructor won't
be called to free the data, so leaking it.

There are other cases that could be okay or not, and may or may not be
consistent with the ResourceObj copy-assign assertion.  I think the
debug-only nesting checking done by GrowableArray is likely more
useful and correct than that ResourceObj assertion.

I looked at a few actual uses.  None were fatally wrong, but none were
good either.  Some looked accidental.

>> Unfortunately, there are already many uses of ResourceObj assignment.
>> I spent a bit of time exploring the idea of removing or replacing it,
>> but that seems way too hard for the intended scope of this cleanup.
>> (Note that for some of the existing uses it is not obvious whether the
>> restriction applies. Presumably it doesn't, since we're not hitting
>> the assert.) While I like the idea of prohibiting assignment (and
>> possibly copy too, which has many of the same issues), I'm not going
>> to pursue that beyond perhaps an RFE for future work.
>> BTW, as part of that exploration I found such gems as this, from
>> opto/cfgnode.cpp:962:
>>   Node_Array node_map = new Node_Array(a);
> 
> I can certainly understand people being confused about C++ allocation, construction and assignment. So I think the above acts as:
> 
> Node_Array node_map;  // Stack object using default constructor
> node_map.operator=(new Node_Array(a));
> 
> Which means the whole "new Node_Array(a)" is completely useless (does it cause a leak?) as operator= ignores it.

Nope.  It is equivalent to

  Node_Array node_map(new Node_Array(a));

No operator= involved at all.  Rather, node_map is a stack-allocated
Node_Array constructed via the conversion (not copy!) constructor
Node_Array(Node_Array*).  That constructor default constructs the
ResourceObj base class object and initializes the Node_Array members
from the corresponding members of the argument object.

The result of "new Node_Array(a)" is then dropped.  Since it's a
ResourceObj, it was allocated in the resource area and will be
reclaimed when the innermost enclosing ResourceMark is exited.

A better way to write this would be

  Node_Array node_map(a);


>> Anyway, even though I think the assert prevents some things that would
>> work (but should perhaps be avoided anyway), and doesn't prevent some
>> things that won't work, I'm willing to reinstate it in order to make
>> progress.
>> New webrevs:
>> full: http://cr.openjdk.java.net/~kbarrett/8213481/open.01/
>> incr: http://cr.openjdk.java.net/~kbarrett/8213481/open.01.inc/
> 
> Thanks for doing that.

Anyway, this has gone somewhat astray from this particular change.
I'm not sure where I am, review-wise.  I *think* both David and
Vladimir are okay with the latest version?