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

[Daniel D. Daugherty]

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