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

Thu Dec 13 23:49:03 UTC 2018

This last version is fine.
Coleen

On 12/6/18 5:04 PM, Kim Barrett wrote:
>> 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?
>