RFR 8082782: vm crash on StressRedefineWithoutBytecodeCorruption fails with assert(((Metadata*)obj)->is_, valid()) failed: obj is valid

[Coleen Phillimore]

Dan, it's apparently in sections 6.6 of the ISO C++ standard which I 
guess we have to buy.  I couldn't find it in the other web page.

http://stackoverflow.com/questions/14688285/c-local-variable-destruction-order

Coleen

On 7/23/15 12:08 PM, Daniel D. Daugherty wrote:
> And one more reply...
>
>
> On 7/23/15 9:52 AM, Coleen Phillimore wrote:
>>
>> One comment on your one comment...
>>
>> On 7/23/15 9:04 AM, Daniel D. Daugherty wrote:
>>> Just one thing below...
>>>
>>>
>>> On 7/22/15 7:25 PM, Coleen Phillimore wrote:
>>>>
>>>> Dan,
>>>>
>>>> Thank you for the quick code review.
>>>>
>>>> On 7/22/15 8:35 PM, Daniel D. Daugherty wrote:
>>>>> On 7/22/15 10:20 AM, Coleen Phillimore wrote:
>>>>>> Summary: Walk compile task for Method* to not deallocate, store 
>>>>>> methods in methodHandle while compile task is being created
>>>>>
>>>>> Wait... 'while compile task is being created'...
>>>>> You're holding these handles while the compile task
>>>>> is being removed... Am I confused here?
>>>>>
>>>>
>>>> No, you're not confused, you are right, we're getting the compile 
>>>> task is being taken off the compile queue.
>>>>>
>>>>>> I couldn't reproduce this or write a test with the exact timing 
>>>>>> needed to get this crash, but this is what the stack trace and 
>>>>>> code inspection tells me what happened. Reran all redefinition 
>>>>>> and internal tonga 'quick' tests on the change with no regression.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8082782.01/
>>>>>
>>>>> src/share/vm/compiler/compileBroker.hpp
>>>>>     No comments.
>>>>>
>>>>> src/share/vm/compiler/compileBroker.cpp
>>>>>     Is there a reason to have save_method and save_hot_method
>>>>>     defined all the way up at lines 671-672 instead of
>>>>>     being defined and init'ed on lines 710-711?
>>>>
>>>> The methodHandle destructor has to run after the mutexLocker 
>>>> destructor.   The methodHandle destructors take it off the list of 
>>>> Methods to mark on stack to not deallocate, but have to run after 
>>>> the MutexLocker destructor so it has to be declared first.
>>>
>>> Ahhh... That deserves a comment, but...
>>> I ran into a case before where I needed:
>>>
>>>    definition objectA;
>>>    definition objectB;
>>>
>>>    <other code>
>>>    definition ret_value = something;
>>>
>>>    return ret_value
>>>    <objectB destructor>
>>>    <objectA destructor>
>>>
>>> but I couldn't find anything in C++ that guaranteed that the
>>> destructors are run in the opposite order of definition.
>>>
>>> What I ended up doing was this:
>>>
>>>    definition objectA;
>>>    definition ret_value;
>>>    { // make sure objectB is destructed before objectA
>>>      definition objectB;
>>>
>>>      <other code>
>>>      ret_value = something;
>>>    } <objectB destructor>
>>>
>>>    return ret_value
>>>    <objectA destructor>
>>>
>>> You might have better luck finding such a C++ guarantee,
>>> but if not...
>>
>> It's in the C++ standard and guaranteed.  No {} are required, 
>> especially for your case.  For the case I have with assignment, it 
>> could go wrong if the assignment operator is defined incorrectly (but 
>> I believe methodHandle assignment operator is correct).
>>
>> http://en.cppreference.com/w/cpp/language/destructor
>
> I'm guessing that this is the wording you're referencing:
>
> > Destruction sequence
> >
> > For both user-defined or implicitly-defined destructors, after
> > the body of the destructor is executed, the compiler calls the
> > destructors for all non-static non-variant members of the class,
> > in reverse order of declaration, then it calls the destructors
> > of all direct base classes in reverse order of construction
> > (which in turn call the destructors of their members and their
> > base classes, etc), and then, if this object is of most-derived
> > class, it calls the destructors of all virtual bases.
> >
> > Even when the destructor is called directly (e.g. obj.~Foo();),
> > the return statement in ~Foo() does not return control to the
> > caller immediately: it calls all those member and base
> > destructors first.
>
>
> The above wording applies to a class, its superclass and
> the members of the class. I don't see any wording about
> unrelated variables of different types... The above phrase
> "in reverse order of declaration" is used when talking
> about members of the class.
>
> I don't see any wording here that guarantee a particular order
> with respect to order of local variable declarations. It could
> be buried there somewhere else, but I don't see it.
>
> Dan
>
>
>>
>> Coleen
>>>
>>> Dan
>>>
>>>>>
>>>>>     L708:   // Save method pointer across
>>>>>         Typo: 'pointer' -> 'pointers'
>>>>
>>>> Fixed.
>>>>>
>>>>>     So the idea of this fix is that you're holding methodHandles for
>>>>>     these method pointers while removing the task because removing
>>>>>     the task can go to a safepoint. While we're at that safepoint,
>>>>>     a RedefineClasses call can come along and try and access/use the
>>>>>     method pointers. 
>>>>
>>>> RedefineClasses will walk the previous versions of the method's 
>>>> holder class and determine that nothing is using the old methods, 
>>>> and put them on the list to deallocate the next time we unload 
>>>> classes.   The methodHandle marks these methods as being used 
>>>> (on_stack) across these safepoints so they stick around.
>>>>
>>>>> If we didn't have methodHandles on them, they
>>>>>     could be freed by the time RedefineClasses tries to access/use
>>>>>     them...
>>>>>
>>>>
>>>> They'd be freed by the time the following code uses them (if you 
>>>> redefine the class and subsequently do some class unloading GC, not 
>>>> terribly likely but can happen in this small window and seems to 
>>>> have happened in this crash from the dump).  It's like a naked 
>>>> methodOop before permgen elimination, except it doesn't move, it 
>>>> gets deallocated.
>>>>
>>>> The is_valid() debugging function partially detects that something 
>>>> has written over this bit of the metadata. Deallocation writes 0xf5 
>>>> over the stuff it deallocates.
>>>>
>>>>> Do I have this right? Ouch and nice find.
>>>>>
>>>> thanks!
>>>>> src/share/vm/runtime/thread.cpp
>>>>>     No comments.
>>>>>
>>>>>
>>>>
>>>> Thank you!!
>>>> Coleen
>>>>
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8082782
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>
>>>>
>>>
>>
>