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

[Daniel D. Daugherty]

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