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