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

[Tom Rodriguez]

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.