RFR 8082782: vm crash on StressRedefineWithoutBytecodeCorruption fails with assert(((Metadata*)obj)->is_, valid()) failed: obj is valid
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jul 23 20:55:23 UTC 2015
Score! Ref:
The C++ Programming Language, 3rd Edition
Bjarne Stroustrup
Section 10.4.4 Local Variables, page 245
... Destructors for local variables are executed in reverse
order of their construction. ...
The index for said book did not have a cross reference for
local variable in the "destructor" section of the index. However,
in the "local variable" section of the index, it did have a cross
reference for "constuctor" on page 245 which led me to the above.
I also have "The Annotated C++ Reference Manual" which is an ANSI
base document. I can't find similar wording there, but that standard
is illegible to humans... :-)
Dan
On 7/23/15 12:21 PM, Coleen Phillimore wrote:
>
> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list