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" <tom.rodriguez at oracle.com> 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"<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
    >
    >