RFR 8163127: Debugger classExclusionFilter does not work correctly with method references

[Daniil Titov]

Hi Chris,

Please review a new version of the patch that moves the disabling of the single stepping into ConstantPool::klass_at_impl().

Mach5 jdk_jdi, vmTestbase_nsk_jdi, vmTestbase_nsk_jdb and serviceability tests, as well as all tier1,tier2 and tier3 tests successfully passed.

Webrev: http://cr.openjdk.java.net/~dtitov/8163127/webrev.03/
Bug: https://bugs.openjdk.java.net/browse/JDK-8163127
   
Thanks!
--Daniil

On 1/24/19, 11:19 AM, "Chris Plummer" <chris.plummer at oracle.com> wrote:

    Hi Daniil,
    
    Thanks for the stack track. I was just about to send an email asking for 
    it when your new RFR arrived.
    
    The fix looks good. I think you also need to apply it here:
    
    InterpreterRuntime::ldc()
    InterpreterRuntime::anewarray()
    InterpreterRuntime::multianewarray()
    InterpreterRuntime::quicken_io_cc()
    
    I wonder if it wouldn't be better if you moved the disabling into 
    ConstantPool::klass_at_impl()
    
    thanks,
    
    Chris
    
    On 1/24/19 10:38 AM, Daniil Titov wrote:
    > Hi Chris and JC,
    >
    > Thank you for reviewing this change.  Please review a new version of the fix that uses
    > the approach Chris suggested ( disabling the single stepping during the class resolution).
    >
    > Just in case please find below the stack trace for this case when loadClass() method is entered.
    >
    > #0           SystemDictionary::load_instance_class(Symbol*, Handle, Thread*) at  open/src/hotspot/share/classfile/systemDictionary.cpp:1502
    > #1	SystemDictionary::resolve_instance_class_or_null(Symbol*, Handle, Handle, Thread*) at open/src/hotspot/share/classfile/systemDictionary.cpp:853
    > #2	SystemDictionary::resolve_instance_class_or_null_helper(Symbol*, Handle, Handle, Thread*) at open/src/hotspot/share/classfile/systemDictionary.cpp:271
    > #3	SystemDictionary::resolve_or_null(Symbol*, Handle, Handle, Thread*) at open/src/hotspot/share/classfile/systemDictionary.cpp:254
    > #4	SystemDictionary::resolve_or_fail(Symbol*, Handle, Handle, bool, Thread*) at open/src/hotspot/share/classfile/systemDictionary.cpp:202
    > #5	ConstantPool::klass_at_impl(constantPoolHandle const&, int, bool, Thread*) at open/src/hotspot/share/oops/constantPool.cpp:483
    > #6	ConstantPool::klass_at(int, Thread*) at open/src/hotspot/share/oops/constantPool.hpp:382
    > #7	InterpreterRuntime::_new(JavaThread*, ConstantPool*, int) at open/src/hotspot/share/interpreter/interpreterRuntime.cpp:234
    > # 8         <Stub Code>
    >   ....
    >
    > Webrev: http://cr.openjdk.java.net/~dtitov/8163127/webrev.02/
    > Bug: https://bugs.openjdk.java.net/browse/JDK-8163127
    >
    > Thanks,
    > Daniil
    >
    > On 1/23/19, 3:53 PM, "Chris Plummer" <chris.plummer at oracle.com> wrote:
    >
    >      Hi Daniil,
    >      
    >      I don't see an explanation for why fromDepth is 1 and afterPopDepth is 4.
    >      
    >               currentDepth = getThreadFrameCount(thread);
    >               fromDepth = step->fromStackDepth;
    >               afterPopDepth = currentDepth-1;
    >      
    >      step->fromStackDepth got setup when single stepping was first setup for
    >      this thread. There was also a notifyFramePop() done at this time, but I
    >      think that's just to catch exiting from the method you were single
    >      stepping in, and has no bearing in the case we are looking at here,
    >      where we area still some # of frames below where we user last issued a
    >      STEP_INTO. The FRAME_POP we are receiving now is not the one for when
    >      step->fromStackDepth was setup, but is for when we stepped into a
    >      filtered method. I think this is what the "fromDepth > afterPopDepth"
    >      check is for. I think the current logic is correct for intended handling
    >      of a FRAME_POP event. Although your fix is probably solving the problem,
    >      I get the feeling it is enabling single stepping too soon in many cases.
    >      That many not turn up as an error in any tests, but could cause
    >      debugging performance issues, or for the user to see spurious single
    >      step events that they were not expecting.
    >      
    >      I think the bug actually occurs long before we ever get to this point in
    >      the code (and we should in fact not be getting here). In my last entry
    >      in the bug I mentioned JvmtiHideSingleStepping(), and how it is used to
    >      turn off single stepping while we are doing invoke and field resolution,
    >      but doesn't seem to be used during class resolution, which is what we
    >      are doing here. If it where used, then the agent would never even see
    >      the SINGLE_STEP when loadClass() is entered, therefore no
    >      notifyFramePop() would be done, and we would not be relying on this code
    >      in handleFramePopEvent(). Instead, we would receive the next SINGLE_STEP
    >      event after cp resolution is complete, and we are finally executing the
    >      now resolved opc_new opcode.
    >      
    >      I'm hoping Serguei and/or Alex can also comment on this, since I think
    >      they were dealing with JvmtiHideSingleStepping() last month.
    >      
    >      thanks,
    >      
    >      Chris
    >      
    >      
    >      
    >      On 1/17/19 6:08 PM, Daniil Titov wrote:
    >      > Please review the change that fixes JDB stepping issue for a specific case when the single step request was initiated earlier in the stack, previous calls were for methods in the filtered classes (single stepping was disabled), handleMethodEnterEvent() re-enabled stepping and the first bytecode upon entering the current method requires resolving constant pool entry. In this case the execution resumes in java.lang.Classloader.loadClass() and since it is also a filtered class the single stepping is getting disabled again (stepControl.c :593).  When loadClass() exits a notifyFramePop() is called on the loadClass() frame but due to condition fromDepth >= afterPopDepth  at stepControl.c :346 (that doesn't hold in this case, in this case fromDepth is 1 and afterPopDepth  is 4) the notifyFramePop() fails to enable single stepping back. The fix removes the excessive condition fromDepth >= afterPopDepth  in notifyFramePop() method (stepControl.c:346)  to ensure that when a method cal!
    >      >   led from the stepping frame (and during which we had stepping disabled) has returned the stepping is re-enabled to continue  instructions steps in the original stepping frame.
    >      >
    >      > Webrev: http://cr.openjdk.java.net/~dtitov/8163127/webrev.01
    >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8163127
    >      >
    >      > Thanks!
    >      > --Daniil
    >      >
    >      >
    >      
    >      
    >      
    >
    >