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

[Daniil Titov]

Hi Serguei,

The testing is not fully completed yet. I ran locally jdk_jdi, vmTestbase_nsk_jdi, vmTestbase_nsk_jdb and have the same tests plus serviceability still running in Mach5. I am also starting tier1,tier2 and tier3 jobs.

Best regards,
Daniil

On 1/24/19, 12:22 PM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:

    Hi Daniil,
    
    I wonder what tests do you run to make sure nothing is broken.
    
    Thanks,
    Serguei
    
    
    On 1/24/19 11:19, Chris Plummer 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
    >>      >
    >>      >
    >>
    >>
    >
    >