review (S) for 6974176: ShouldNotReachHere, instanceKlass.cpp:1426

[Vladimir Kozlov]

Tom,

Should the next code be in own scope {} ?:

       // zombie only - if a JVMTI agent has enabled the CompiledMethodUnload event
       // and it hasn't already been reported for this nmethod then report it now.
       // (the event may have been reported earilier if the GC marked it for unloading).
+     Pause_No_Safepoint_Verifier pnsv(&nsv);
       post_compiled_method_unload();

       MutexLockerEx mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
       flush_dependencies(NULL);
+ #ifdef ASSERT
+     // It's no longer safe to access the oops section since zombie
+     // nmethods aren't scanned for GC.
+     _oops_are_stale = true;
+ #endif

Thanks,
Vladimir

Tom Rodriguez wrote:
> http://cr.openjdk.java.net/~never/6974176
> 
> 6974176: ShouldNotReachHere, instanceKlass.cpp:1426
> Reviewed-by:
> 
> The changes for 6965184 reordered the flush_dependencies and
> post_compiled_method_unload which allowed a safepoint to happen in
> between.  Zombie nmethods don't have their oops scanned so when
> flush_dependencies was run it was reading stale oops.  If the oops
> didn't move then it would work as intended but otherwise you might get
> various weird failures.  The fix is to restore the original order.  I
> also added a No_SafePoint_Verifier and some logic to mark the oops as
> potentially stale.  Tested with failing test on SQE machine that
> reproduced it reliably, plus nsk suites with -XX:+DeoptimizeALot
> -XX:CompileThreshold=100.