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

[Vladimir Kozlov]

Looks good.

Vladimir

Tom Rodriguez wrote:
> I could explicitly wrap it but I figured it was ok since it terminated the scope.  It's probably clearer if I wrap it.  (copy paste made that code look wrong, since the flush_dep call has actually been moved).  I've updated the webrev.
> 
> tom
> 
> On Aug 11, 2010, at 12:13 PM, Vladimir Kozlov wrote:
> 
>> 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.
>