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

[Daniel D. Daugherty]

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