RFR 8163127: Debugger classExclusionFilter does not work correctly with method references
Jean Christophe Beyler
jcbeyler at google.com
Tue Jan 29 20:09:38 UTC 2019
Hi Daniil,
I like this fix much better to be honest :)
Jc
On Tue, Jan 29, 2019 at 11:40 AM Daniil Titov <daniil.x.titov at oracle.com>
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" <chris.plummer at oracle.com> 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" <chris.plummer at oracle.com>
> 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
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
>
>
>
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190129/288c6a87/attachment-0001.html>
More information about the serviceability-dev
mailing list