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

[serguei.spitsyn at oracle.com]

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
>>      >
>>      >
>>
>>
>
>