RFR 8192004: InspectedFrame.materializeVirtualObjects only updates locals with new objects

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR 8192004: InspectedFrame.materializeVirtualObjects only updates locals with new objects

Tom Rodriguez-2
JVMCI adds the ability to introspect on deoptimized frames which
requires early materialization of escape analyzed objects.  The
jvmtiDeferredLocalVariableSet machinery is reused to capture the local
updates required for this.  The existing code only updates locals,
leaving the stack and monitor information with out of date values.  This
can lead to deadlocks and incorrect execution.  The fix is to slightly
generalize jvmtiDeferredLocalVariableSet to handle expression stack and
monitor slots.  Tested with new graal regression test
https://github.com/graalvm/graal/blob/7fd37bde8955780a57049964d87a51aa2407d86b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotStackIntrospectionTest.java 
and previously failing Truffle use cases.  I assume the new test case
will come across with a normal graal update.  The clean mach5 run is in
the bug report.

http://cr.openjdk.java.net/~never/8192004/webrev
https://bugs.openjdk.java.net/browse/JDK-8192004
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192004: InspectedFrame.materializeVirtualObjects only updates locals with new objects

Tom Rodriguez-2
Could I get some reviews?  This doesn't change the way the logic behaves
for the core use of JVMTI.  It just extends the encoding so that numbers
after the locals are interpreted as expression stack and monitor values.
  I believe there are existing tests of the JVMTI set locals part and
JVMCI is the only only consumer of the monitor and expression stack part.

tom

Tom Rodriguez wrote:

> JVMCI adds the ability to introspect on deoptimized frames which
> requires early materialization of escape analyzed objects. The
> jvmtiDeferredLocalVariableSet machinery is reused to capture the local
> updates required for this. The existing code only updates locals,
> leaving the stack and monitor information with out of date values. This
> can lead to deadlocks and incorrect execution. The fix is to slightly
> generalize jvmtiDeferredLocalVariableSet to handle expression stack and
> monitor slots. Tested with new graal regression test
> https://github.com/graalvm/graal/blob/7fd37bde8955780a57049964d87a51aa2407d86b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotStackIntrospectionTest.java
> and previously failing Truffle use cases. I assume the new test case
> will come across with a normal graal update. The clean mach5 run is in
> the bug report.
>
> http://cr.openjdk.java.net/~never/8192004/webrev
> https://bugs.openjdk.java.net/browse/JDK-8192004
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192004: InspectedFrame.materializeVirtualObjects only updates locals with new objects

Hohensee, Paul
In vframe_hp.cpp:

In jvmtiDeferredocalVariableSet::update_monitors, the calculation of lock_index

      int lock_index = val->index() - (method()->max_locals() + method()->max_stack());

looks wrong. If lock_index is supposed to be zero-based, then method()->max_stack() should be subtracted from val->index(), vis

      int lock_index = val->index() - (method()->max_locals() - method()->max_stack());

A terrifically minor nit, but using ‘l’ as the induction variable instead of ‘i’ is a bit odd. (

Otherwise fine.

Thanks,

Paul

On 12/22/17, 11:09 AM, "hotspot-compiler-dev on behalf of Tom Rodriguez" <[hidden email] on behalf of [hidden email]> wrote:

    Could I get some reviews?  This doesn't change the way the logic behaves
    for the core use of JVMTI.  It just extends the encoding so that numbers
    after the locals are interpreted as expression stack and monitor values.
      I believe there are existing tests of the JVMTI set locals part and
    JVMCI is the only only consumer of the monitor and expression stack part.
   
    tom
   
    Tom Rodriguez wrote:
    > JVMCI adds the ability to introspect on deoptimized frames which
    > requires early materialization of escape analyzed objects. The
    > jvmtiDeferredLocalVariableSet machinery is reused to capture the local
    > updates required for this. The existing code only updates locals,
    > leaving the stack and monitor information with out of date values. This
    > can lead to deadlocks and incorrect execution. The fix is to slightly
    > generalize jvmtiDeferredLocalVariableSet to handle expression stack and
    > monitor slots. Tested with new graal regression test
    > https://github.com/graalvm/graal/blob/7fd37bde8955780a57049964d87a51aa2407d86b/compiler/src/org.graalvm.compiler.hotspot.test/src/org/graalvm/compiler/hotspot/test/HotSpotStackIntrospectionTest.java
    > and previously failing Truffle use cases. I assume the new test case
    > will come across with a normal graal update. The clean mach5 run is in
    > the bug report.
    >
    > http://cr.openjdk.java.net/~never/8192004/webrev
    > https://bugs.openjdk.java.net/browse/JDK-8192004
   

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192004: InspectedFrame.materializeVirtualObjects only updates locals with new objects

Tom Rodriguez-2


Hohensee, Paul wrote:
> In vframe_hp.cpp:
>
> In jvmtiDeferredocalVariableSet::update_monitors, the calculation of lock_index
>
>        int lock_index = val->index() - (method()->max_locals() + method()->max_stack());
>
> looks wrong. If lock_index is supposed to be zero-based, then method()->max_stack() should be subtracted from val->index(), vis
>
>        int lock_index = val->index() - (method()->max_locals() - method()->max_stack());

The whole point is to linearize the numbering of locals + stack +
monitors.  The locks are in the last section so we need to remove locals
+ stack.  Did you miss the parentheses?

>
> A terrifically minor nit, but using ‘l’ as the induction variable instead of ‘i’ is a bit odd. (

That is minor.  :) It's l for locals and comes from the original code
which only operates on locals.  I think I'll leave it.

tom

>
> Otherwise fine.
>
> Thanks,
>
> Paul
>
> On 12/22/17, 11:09 AM, "hotspot-compiler-dev on behalf of Tom Rodriguez"<[hidden email] on behalf of [hidden email]>  wrote:
>
>      Could I get some reviews?  This doesn't change the way the logic behaves
>      for the core use of JVMTI.  It just extends the encoding so that numbers
>      after the locals are interpreted as expression stack and monitor values.
>        I believe there are existing tests of the JVMTI set locals part and
>      JVMCI is the only only consumer of the monitor and expression stack part.
>
>      tom
>
>      Tom Rodriguez wrote:
>      >  JVMCI adds the ability to introspect on deoptimized frames which
>      >  requires early materialization of escape analyzed objects. The
>      >  jvmtiDeferredLocalVariableSet machinery is reused to capture the local
>      >  updates required for this. The existing code only updates locals,
>      >  leaving the stack and monitor information with out of date values. This
>      >  can lead to deadlocks and incorrect execution. The fix is to slightly
>      >  generalize jvmtiDeferredLocalVariableSet to handle expression stack and
>      >  monitor slots. Tested with new graal regression test
>      >  https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_graalvm_graal_blob_7fd37bde8955780a57049964d87a51aa2407d86b_compiler_src_org.graalvm.compiler.hotspot.test_src_org_graalvm_compiler_hotspot_test_HotSpotStackIntrospectionTest.java&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=z7nIdItnZDaCCWKkUI4TIwrvK5oo0Gcyu76zdeNUJ6o&m=bEBUxT7opUyW4i5zri3c18nkJeDzKdf3Xg8dA7KCgmQ&s=NnzTxQnCMbtxi8DNI18Ylx7FoXBtP9s0y3kNeEcyCHE&e=
>      >  and previously failing Truffle use cases. I assume the new test case
>      >  will come across with a normal graal update. The clean mach5 run is in
>      >  the bug report.
>      >
>      >  http://cr.openjdk.java.net/~never/8192004/webrev
>      >  https://bugs.openjdk.java.net/browse/JDK-8192004
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8192004: InspectedFrame.materializeVirtualObjects only updates locals with new objects

Hohensee, Paul
Plainly I missed the parens.

Thanks,

Paul

On 1/18/18, 8:56 AM, "Tom Rodriguez" <[hidden email]> wrote:

   
   
    Hohensee, Paul wrote:
    > In vframe_hp.cpp:
    >
    > In jvmtiDeferredocalVariableSet::update_monitors, the calculation of lock_index
    >
    >        int lock_index = val->index() - (method()->max_locals() + method()->max_stack());
    >
    > looks wrong. If lock_index is supposed to be zero-based, then method()->max_stack() should be subtracted from val->index(), vis
    >
    >        int lock_index = val->index() - (method()->max_locals() - method()->max_stack());
   
    The whole point is to linearize the numbering of locals + stack +
    monitors.  The locks are in the last section so we need to remove locals
    + stack.  Did you miss the parentheses?
   
    >
    > A terrifically minor nit, but using ‘l’ as the induction variable instead of ‘i’ is a bit odd. (
   
    That is minor.  :) It's l for locals and comes from the original code
    which only operates on locals.  I think I'll leave it.
   
    tom
   
    >
    > Otherwise fine.
    >
    > Thanks,
    >
    > Paul
    >
    > On 12/22/17, 11:09 AM, "hotspot-compiler-dev on behalf of Tom Rodriguez"<[hidden email] on behalf of [hidden email]>  wrote:
    >
    >      Could I get some reviews?  This doesn't change the way the logic behaves
    >      for the core use of JVMTI.  It just extends the encoding so that numbers
    >      after the locals are interpreted as expression stack and monitor values.
    >        I believe there are existing tests of the JVMTI set locals part and
    >      JVMCI is the only only consumer of the monitor and expression stack part.
    >
    >      tom
    >
    >      Tom Rodriguez wrote:
    >      >  JVMCI adds the ability to introspect on deoptimized frames which
    >      >  requires early materialization of escape analyzed objects. The
    >      >  jvmtiDeferredLocalVariableSet machinery is reused to capture the local
    >      >  updates required for this. The existing code only updates locals,
    >      >  leaving the stack and monitor information with out of date values. This
    >      >  can lead to deadlocks and incorrect execution. The fix is to slightly
    >      >  generalize jvmtiDeferredLocalVariableSet to handle expression stack and
    >      >  monitor slots. Tested with new graal regression test
    >      >  https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_graalvm_graal_blob_7fd37bde8955780a57049964d87a51aa2407d86b_compiler_src_org.graalvm.compiler.hotspot.test_src_org_graalvm_compiler_hotspot_test_HotSpotStackIntrospectionTest.java&d=DwIGaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=z7nIdItnZDaCCWKkUI4TIwrvK5oo0Gcyu76zdeNUJ6o&m=bEBUxT7opUyW4i5zri3c18nkJeDzKdf3Xg8dA7KCgmQ&s=NnzTxQnCMbtxi8DNI18Ylx7FoXBtP9s0y3kNeEcyCHE&e=
    >      >  and previously failing Truffle use cases. I assume the new test case
    >      >  will come across with a normal graal update. The clean mach5 run is in
    >      >  the bug report.
    >      >
    >      >  http://cr.openjdk.java.net/~never/8192004/webrev
    >      >  https://bugs.openjdk.java.net/browse/JDK-8192004
    >
    >