Thank you Chris and JC for reviewing this change.
Best regards, Daniil From: Jean Christophe Beyler <[email protected]> Date: Tuesday, January 29, 2019 at 12:09 PM To: Daniil Titov <[email protected]> Cc: OpenJDK Serviceability <[email protected]>, Chris Plummer <[email protected]> Subject: Re: RFR 8163127: Debugger classExclusionFilter does not work correctly with method references Hi Daniil, I like this fix much better to be honest :) Jc On Tue, Jan 29, 2019 at 11:40 AM Daniil Titov <[email protected]> wrote: Hi JC, Could you please say are you OK with this new version of the fix? Thanks! --Daniil On 1/26/19, 11:35 AM, "Chris Plummer" <[email protected]> wrote: Looks good. thanks, Chris On 1/26/19 11:23 AM, Daniil Titov wrote: > 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" <[email protected]> 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" <[email protected]> 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 > > > > > > > > > > > > > > > > > > > > > -- Thanks, Jc
