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

[David Holmes]

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=

ResourceObj is not a nice clean, well-defined class - 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.

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

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

David
-----