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

[Coleen Phillimore]

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

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