RFR (s) 8148772: VM crash in nsk/jvmti/RedefineClasses/StressRedefine: assert failed: Corrupted constant pool

[Coleen Phillimore]

Thanks, Dan.   I also commented in the bug.

Can you review 8151546 also?  I want to fix them together since they 
together fixed the test (which I don't think is quarantined in any way - 
I should check).

Thanks!
Coleen

On 4/12/16 6:36 PM, Daniel D. Daugherty wrote:
> OK, I think I understand the code. Please see the note that I added
> to the bug report. I have a couple of final questions about the
> other failures modes in JDK-8148772, but I suspect those are covered
> by the work on:
>
> JDK-8151546 nsk/jvmti/RedefineClasses/StressRedefine fails in hs nightly
>
> Thumbs up on this code.
>
> Dan
>
>
> On 4/9/16 7:05 AM, Coleen Phillimore wrote:
>>
>> Hi Dan,  I tried to answer your questions in the comments of the bug 
>> so there'd be a record (at least for me).  I wasn't very descriptive 
>> in my earlier comment, because fixing this bug was prelude to trying 
>> to fix another bug with this StressRedefine test case.
>>
>> On 4/8/16 9:46 PM, Daniel D. Daugherty wrote:
>>> On 4/8/16 3:06 PM, Coleen Phillimore wrote:
>>>> Summary: ConstantPool::resolve_constant_at_impl() isn't thread safe 
>>>> for MethodHandleInError and MethodTypeInError.
>>>>
>>>> Need to ignore the InError tag when fetching method_handle_index 
>>>> and method_type_index.  The error is cached after the call to 
>>>> systemDictionary::link_method_handle_constant() if it's not there 
>>>> already.
>>>>
>>>> Tested with rbt equivalent of nightly runs, and StressRedefine test 
>>>> (reproduceable with this error) for >24 hours (also with 8151546 
>>>> fixed).  Ran jdk/test/java/lang/invoke tests.  I can't write a test 
>>>> for this because it's too timing sensitive.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8148772.01/webrev
>>>
>>> I'm trying to get my head wrapped around this race...
>>> so the original failure mode looks like this:
>>>
>>>      assert(tag_at(which).is_invoke_dynamic()) failed: Corrupted 
>>> constant pool
>>>
>>> and the call stack looks like this:
>>>
>>> V  [libjvm.so+0x7f1fe0]  report_vm_error(char const*, int, char 
>>> const*, char const*, ...)+0x60
>>> V  [libjvm.so+0x7e518b] 
>>> ConstantPool::invoke_dynamic_name_and_type_ref_index_at(int)+0x3b
>>> V  [libjvm.so+0x7dd18f] 
>>> ConstantPool::impl_name_and_type_ref_index_at(int, bool)+0x15f
>>> V  [libjvm.so+0x6a7363] 
>>> ciBytecodeStream::get_method_signature_index()+0x4a3
>>>
>>> and the crashing code looks like this:
>>>
>>>  517   int invoke_dynamic_name_and_type_ref_index_at(int which) {
>>>  518     assert(tag_at(which).is_invoke_dynamic(), "Corrupted 
>>> constant pool");
>>>  519     return extract_high_short_from_int(*int_at_addr(which));
>>>  520   }
>>>
>>> The other crashes in the bug report are different and are in
>>> different places... I don't think I'm going to get there by
>>> looking at the reported crashes...
>>>
>>> OK, so the bug report has one line of analysis:
>>>
>>> > ConstantPool::resolve_constant_at_impl() isn't thread safe for
>>> > MethodHandleInError and MethodTypeInError.
>>>
>>> but resolve_constant_at_impl() isn't changed at all by the webrev.
>>> OK, this is starting to get frustrating...
>>>
>>> OK, so I go back to the code and look at it again...
>>> The constantPool.hpp changes are all about getting
>>> rid of the 'error_ok' parameter and getting rid of
>>> the _error_ok() function variants. I'm cool with all
>>> that code, but I don't see what it has to do with a
>>> data race in the constant pool...
>>>
>>> The constantPool.cpp changes are all about switching
>>> from the _error_ok() function variants to regular
>>> variants. And there's the new debug additions to
>>> invalid/default part of the case statement... I'm
>>> still not seeing it...
>>>
>>> So since the constantPool.cpp code that used to call
>>> the _error_ok() functions now call the regular functions
>>> that means that this race has to be in the original
>>> functions that took the error_ok parameter... so I
>>> look again and I just don't see how removing the
>>> error_ok parameter and its use in the asserts() solves
>>> this race.
>>>
>>> OK, it's late on a Friday and I'm just not getting
>>> what this fix is about...
>>>
>>> src/share/vm/oops/constantPool.hpp
>>>     No comments.
>>>
>>> src/share/vm/oops/constantPool.cpp
>>>     L1024:     DEBUG_ONLY( tty->print_cr("*** %p: tag at CP[%d] = %d",
>>>     L1025:                               this, index1, t1));
>>>     L1026:     assert(false, "unexpected constant tag");
>>>     L1028:     ShouldNotReachHere();
>>>         I agree with Chris that this should be merged into
>>>         a fatal() call. Should the '%p' be a INTPTR_FORMAT?
>>>         I have a vague memory about '%p' being problematic
>>>         to get consistent across all platforms.
>>
>> I revered this change.
>>
>> Thanks,
>> Coleen
>>>
>>> I'll look at it again on Monday. For now my review is
>>> about style since I clearly don't understand this race
>>> nor how this fix solves it.
>>>
>>> Dan
>>>
>>>
>>>
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8148772
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>