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