[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 8163127: Debugger classExclusionFilter does not work correctly with method references
From: Daniil Titov <daniil.x.titov () oracle ! com>
Date: 2019-01-29 21:32:03
Message-ID: D98FC5EF-2646-4A28-9CAE-4F1A99EEBC7F () oracle ! com
[Download RAW message or body]
Thank you Chris and JC for reviewing this change.
Best regards,
Daniil
From: Jean Christophe Beyler <jcbeyler@google.com>
Date: Tuesday, January 29, 2019 at 12:09 PM
To: Daniil Titov <daniil.x.titov@oracle.com>
Cc: OpenJDK Serviceability <serviceability-dev@openjdk.java.net>, Chris Plummer \
<chris.plummer@oracle.com>
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 <daniil.x.titov@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@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@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@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
[Attachment #3 (text/html)]
<html xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type \
content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 15 \
(filtered medium)"><style><!-- /* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p \
class=MsoNormal>Thank you Chris and JC for reviewing this change.<o:p></o:p></p><p \
class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Best \
regards,<o:p></o:p></p><p class=MsoNormal>Daniil<o:p></o:p></p><p \
class=MsoNormal><o:p> </o:p></p><div style='border:none;border-top:solid #B5C4DF \
1.0pt;padding:3.0pt 0in 0in 0in'><p class=MsoNormal><b><span \
style='font-size:12.0pt;color:black'>From: </span></b><span \
style='font-size:12.0pt;color:black'>Jean Christophe Beyler \
<jcbeyler@google.com><br><b>Date: </b>Tuesday, January 29, 2019 at 12:09 \
PM<br><b>To: </b>Daniil Titov <daniil.x.titov@oracle.com><br><b>Cc: </b>OpenJDK \
Serviceability <serviceability-dev@openjdk.java.net>, Chris Plummer \
<chris.plummer@oracle.com><br><b>Subject: </b>Re: RFR 8163127: Debugger \
classExclusionFilter does not work correctly with method \
references<o:p></o:p></span></p></div><div><p \
class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Hi \
Daniil,<o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><div><p \
class=MsoNormal>I like this fix much better to be honest \
:)<o:p></o:p></p></div><div><p class=MsoNormal>Jc<o:p></o:p></p></div></div><p \
class=MsoNormal><o:p> </o:p></p><div><div><p class=MsoNormal>On Tue, Jan 29, \
2019 at 11:40 AM Daniil Titov <<a \
href="mailto:daniil.x.titov@oracle.com">daniil.x.titov@oracle.com</a>> \
wrote:<o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC \
1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in'><p \
class=MsoNormal style='margin-bottom:12.0pt'>Hi JC,<br><br>Could you please say are \
you OK with this new version of the fix?<br><br>Thanks!<br>--Daniil<br><br><br>On \
1/26/19, 11:35 AM, "Chris Plummer" <<a \
href="mailto:chris.plummer@oracle.com" \
target="_blank">chris.plummer@oracle.com</a>> wrote:<br><br> Looks \
good.<br><br> thanks,<br><br> Chris<br><br> On \
1/26/19 11:23 AM, Daniil Titov wrote:<br> > Hi Chris,<br> \
><br> > Please review a new version of the patch that moves \
the disabling of the single stepping into ConstantPool::klass_at_impl().<br> \
><br> > Mach5 jdk_jdi, vmTestbase_nsk_jdi, \
vmTestbase_nsk_jdb and serviceability tests, as well as all tier1,tier2 and tier3 \
tests successfully passed.<br> ><br> > Webrev: <a \
href="http://cr.openjdk.java.net/~dtitov/8163127/webrev.03/" \
target="_blank">http://cr.openjdk.java.net/~dtitov/8163127/webrev.03/</a><br> \
> Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8163127" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8163127</a><br> \
> <br> > Thanks!<br> > \
--Daniil<br> ><br> > On 1/24/19, 11:19 AM, \
"Chris Plummer" <<a href="mailto:chris.plummer@oracle.com" \
target="_blank">chris.plummer@oracle.com</a>> wrote:<br> \
><br> > Hi Daniil,<br> > \
<br> > Thanks for the stack track. \
I was just about to send an email asking for<br> > \
it when your new RFR arrived.<br> > \
<br> > The fix looks good. I think you also need \
to apply it here:<br> > <br> \
> InterpreterRuntime::ldc()<br> > \
InterpreterRuntime::anewarray()<br> > \
InterpreterRuntime::multianewarray()<br> > \
InterpreterRuntime::quicken_io_cc()<br> > \
<br> > I wonder if it wouldn't be better if you \
moved the disabling into<br> > \
ConstantPool::klass_at_impl()<br> > <br> \
> thanks,<br> > \
<br> > Chris<br> > \
<br> > On 1/24/19 10:38 AM, Daniil Titov \
wrote:<br> > > Hi Chris and JC,<br> \
> ><br> > > \
Thank you for reviewing this change. Please review a new version of the fix \
that uses<br> > > the approach Chris suggested \
( disabling the single stepping during the class resolution).<br> \
> ><br> > > Just in \
case please find below the stack trace for this case when loadClass() method is \
entered.<br> > ><br> > \
> #0 \
SystemDictionary::load_instance_class(Symbol*, Handle, Thread*) at \
open/src/hotspot/share/classfile/systemDictionary.cpp:1502<br> \
> > #1 \
SystemDictionary::resolve_instance_class_or_null(Symbol*, Handle, Handle, Thread*) at \
open/src/hotspot/share/classfile/systemDictionary.cpp:853<br> > \
> #2 \
SystemDictionary::resolve_instance_class_or_null_helper(Symbol*, Handle, Handle, \
Thread*) at open/src/hotspot/share/classfile/systemDictionary.cpp:271<br> \
> > #3 SystemDictionary::resolve_or_null(Symbol*, \
Handle, Handle, Thread*) at \
open/src/hotspot/share/classfile/systemDictionary.cpp:254<br> > \
> #4 SystemDictionary::resolve_or_fail(Symbol*, Handle, Handle, \
bool, Thread*) at open/src/hotspot/share/classfile/systemDictionary.cpp:202<br> \
> > #5 \
ConstantPool::klass_at_impl(constantPoolHandle const&, int, bool, Thread*) at \
open/src/hotspot/share/oops/constantPool.cpp:483<br> > \
> #6 ConstantPool::klass_at(int, Thread*) at \
open/src/hotspot/share/oops/constantPool.hpp:382<br> > \
> #7 InterpreterRuntime::_new(JavaThread*, ConstantPool*, int) at \
open/src/hotspot/share/interpreter/interpreterRuntime.cpp:234<br> \
> > # 8 <Stub \
Code><br> > > ....<br> \
> ><br> > > \
Webrev: <a href="http://cr.openjdk.java.net/~dtitov/8163127/webrev.02/" \
target="_blank">http://cr.openjdk.java.net/~dtitov/8163127/webrev.02/</a><br> \
> > Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8163127" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8163127</a><br> \
> ><br> > > \
Thanks,<br> > > Daniil<br> \
> ><br> > > On \
1/23/19, 3:53 PM, "Chris Plummer" <<a \
href="mailto:chris.plummer@oracle.com" \
target="_blank">chris.plummer@oracle.com</a>> wrote:<br> > \
><br> > > \
Hi Daniil,<br> > ><br> > \
> I don't see an explanation for why fromDepth \
is 1 and afterPopDepth is 4.<br> > ><br> \
> > \
currentDepth = getThreadFrameCount(thread);<br> > \
> fromDepth = \
step->fromStackDepth;<br> > > \
afterPopDepth = currentDepth-1;<br> \
> ><br> > \
> step->fromStackDepth got setup when single stepping was \
first setup for<br> > > \
this thread. There was also a notifyFramePop() done at this time, but I<br> \
> > think that's just to catch \
exiting from the method you were single<br> > \
> stepping in, and has no bearing in the case we are looking \
at here,<br> > > where we \
area still some # of frames below where we user last issued a<br> \
> > STEP_INTO. The FRAME_POP we are \
receiving now is not the one for when<br> > \
> step->fromStackDepth was setup, but is for when we \
stepped into a<br> > > \
filtered method. I think this is what the "fromDepth > \
afterPopDepth"<br> > > \
check is for. I think the current logic is correct for intended \
handling<br> > > of a \
FRAME_POP event. Although your fix is probably solving the problem,<br> \
> > I get the feeling it is enabling \
single stepping too soon in many cases.<br> > \
> That many not turn up as an error in any tests, but could \
cause<br> > > debugging \
performance issues, or for the user to see spurious single<br> \
> > step events that they were not \
expecting.<br> > ><br> > \
> I think the bug actually occurs long before we \
ever get to this point in<br> > > \
the code (and we should in fact not be getting here). In my last \
entry<br> > > in the bug I \
mentioned JvmtiHideSingleStepping(), and how it is used to<br> \
> > turn off single stepping while we \
are doing invoke and field resolution,<br> > \
> but doesn't seem to be used during class resolution, which \
is what we<br> > > are \
doing here. If it where used, then the agent would never even see<br> \
> > the SINGLE_STEP when loadClass() is \
entered, therefore no<br> > > \
notifyFramePop() would be done, and we would not be relying on this \
code<br> > > in \
handleFramePopEvent(). Instead, we would receive the next SINGLE_STEP<br> \
> > event after cp resolution is \
complete, and we are finally executing the<br> > \
> now resolved opc_new opcode.<br> > \
><br> > > \
I'm hoping Serguei and/or Alex can also comment on this, since I think<br> \
> > they were dealing with \
JvmtiHideSingleStepping() last month.<br> > \
><br> > > \
thanks,<br> > ><br> > \
> Chris<br> > \
><br> > ><br> \
> ><br> > > \
On 1/17/19 6:08 PM, Daniil Titov wrote:<br> > \
> > 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!<br> \
> > > 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.<br> > > \
><br> > > > Webrev: \
<a href="http://cr.openjdk.java.net/~dtitov/8163127/webrev.01" \
target="_blank">http://cr.openjdk.java.net/~dtitov/8163127/webrev.01</a><br> \
> > > Bug: <a \
href="https://bugs.openjdk.java.net/browse/JDK-8163127" \
target="_blank">https://bugs.openjdk.java.net/browse/JDK-8163127</a><br> \
> > ><br> > \
> > Thanks!<br> > \
> > --Daniil<br> > \
> ><br> > \
> ><br> > \
><br> > ><br> > \
><br> > ><br> \
> ><br> > <br> \
> <br> > <br> \
><br> \
><br><br><br><br><br><o:p></o:p></p></blockquote></div><p class=MsoNormal><br \
clear=all><o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><p \
class=MsoNormal>-- <o:p></o:p></p><div><div><div><p \
class=MsoNormal><o:p> </o:p></p></div><p \
class=MsoNormal>Thanks,<o:p></o:p></p><div><p \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic