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
