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

[Tom Rodriguez]

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"<hotspot-compiler-dev-bounces at openjdk.java.net on behalf of tom.rodriguez at oracle.com>  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
>
>