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

[Coleen Phillimore]

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