RFR: Deoptimization bug

[Roman Kennke]

Now I come to think that turning on -XX:+ShenandoahSuspendibleWorkers by
default is the better/best fix. The problem arises because GC workers
don't stop during deopt, and thus the RB in oopDesc::mark() is racy.
With -XX:+ShenandoahSuspendibleWorkers this would not happen.

WDYT?

Roman


> Let's turn this into a proper fix and RFR. I think a better and
> simpler/less intrusive fix is the one below (JDK12, JDK11 and JDK8),
> e.g. WB all oops from deoptimization before entering slow_enter(). All
> other paths to slow_enter() should have the WB already (that only
> happens when coming from interpreter/C1/C2).
> 
> Tests: tier3_gc_shenandoah
> 
> Aleksey, can you verify that it helps our failing tests too? So far I
> failed to setup those lucene tests...
> 
> JDK11/12:
> 
> diff --git a/src/hotspot/share/runtime/deoptimization.cpp
> b/src/hotspot/share/runtime/deoptimization.cpp
> --- a/src/hotspot/share/runtime/deoptimization.cpp
> +++ b/src/hotspot/share/runtime/deoptimization.cpp
> @@ -1098,7 +1098,7 @@
>      if (mon_info->eliminated()) {
>        assert(!mon_info->owner_is_scalar_replaced() || realloc_failures,
> "reallocation was missed");
>        if (!mon_info->owner_is_scalar_replaced()) {
> -        Handle obj(thread, mon_info->owner());
> +        Handle obj(thread, Access<>::resolve(mon_info->owner()));
>          markOop mark = obj->mark();
>          if (UseBiasedLocking && mark->has_bias_pattern()) {
>            // New allocated objects may have the mark set to anonymously
> biased.
> 
> 
> JDK8:
> diff --git a/src/share/vm/runtime/deoptimization.cpp
> b/src/share/vm/runtime/deoptimization.cpp
> --- a/src/share/vm/runtime/deoptimization.cpp
> +++ b/src/share/vm/runtime/deoptimization.cpp
> @@ -990,7 +990,7 @@
>      if (mon_info->eliminated()) {
>        assert(!mon_info->owner_is_scalar_replaced() || realloc_failures,
> "reallocation was missed");
>        if (!mon_info->owner_is_scalar_replaced()) {
> -        Handle obj = Handle(mon_info->owner());
> +        Handle obj =
> Handle(oopDesc::bs()->write_barrier(mon_info->owner()));
>          markOop mark = obj->mark();
>          if (UseBiasedLocking && mark->has_bias_pattern()) {
>            // New allocated objects may have the mark set to anonymously
> biased.
> 
> Roman
> 
> 
> Am 04.10.18 um 21:44 schrieb Roman Kennke:
>> We've observed a rather nasty bug in some tests in jdk8u lately. From
>> what I can tell, this affects all versions though. I am not 100% sure
>> that what I'm thinking is correct, so let me describe it for further
>> discussion.
>>
>> Deoptimization happens at safepoints. It may happen at a safepoint in
>> the middle of concurrent evac. In fact (but this is not really relevant
>> for the fix I think), evac threads might continue to run concurrently
>> with the deopt-safepoint. (We do have an option to make GC workers
>> synchronize at safepoints too, we might consider this.)
>>
>> Deoptimization generates valid interpreter frames out of compiled
>> frames. One of the funny things it needs to do is recreate locks that
>> the compiler has eliminated (via escape analysis). In order to do this,
>> it creates a bunch of compiledVFrame objects that get populated with
>> on-stack-objects (or sometimes new objects, because it also needs to
>> recreate objects that have been eliminated because of escape analysis).
>> Those on-stack-objects may be from-space-copies. If such a
>> from-space-copy is subsequently passed to slow_enter() (to re-create the
>> lock), it will take the from-space-mark and write that into the
>> displaced header. This blows up later, of course.
>>
>> A possible quick fix for this may be:
>> http://cr.openjdk.java.net/~rkennke/fix-locking.patch
>>
>> But I'm not really happy with it. We probably should comb through
>> deoptimization.cpp and find the right places where we need to apply WB.
>>
>> Roman
>>
>