[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR 8193150: Create a jtreg version of the test from JDK-8187143
From: Paru Somashekar <parvathi.somashekar () oracle ! com>
Date: 2018-02-09 2:16:59
Message-ID: 5A7D049B.1010109 () oracle ! com
[Download RAW message or body]
Thanks Chris and Serguei for the review.
thanks,
Paru.
On 2/8/18, 4:49 PM, serguei.spitsyn@oracle.com wrote:
> Hi Paru,
>
> +1
>
> Thanks,
> Serguei
>
>
> On 2/8/18 15:36, Chris Plummer wrote:
>> Hi Paru,
>>
>> Looks good. Thanks for the changes.
>>
>> Chris
>>
>> On 2/8/18 2:50 PM, Paru Somashekar wrote:
>>>
>>> I have incorporated all your feedback and created a new webrev at :
>>> http://cr.openjdk.java.net/~psomashe/8193150/webrev.01/
>>> <http://cr.openjdk.java.net/%7Epsomashe/8193150/webrev.01/>
>>>
>>> - Added comments
>>> - modified the logic for failReason and breakpoint reached aspect on
>>> the debugger side.
>>>
>>> thanks,
>>> Paru.
>>>
>>> On 2/7/18, 6:55 PM, Paru Somashekar wrote:
>>>> Hi Chris, Serguei,
>>>>
>>>> On 2/7/18, 4:56 PM, serguei.spitsyn@oracle.com wrote:
>>>>> On 2/7/18 16:47, Chris Plummer wrote:
>>>>>> On 2/7/18 3:23 PM, serguei.spitsyn@oracle.com
>>>>>> <mailto:serguei.spitsyn@oracle.com> wrote:
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>>
>>>>>>> On 2/7/18 15:06, Chris Plummer wrote:
>>>>>>>> Hi Paru,
>>>>>>>>
>>>>>>>> On 2/7/18 2:30 PM, Paru Somashekar wrote:
>>>>>>>>> Thanks for the review Chris, comments inline..
>>>>>>>>> On 2/7/18, 1:25 PM, Chris Plummer wrote:
>>>>>>>>>> Hi Paru,
>>>>>>>>>>
>>>>>>>>>> Thanks for writing this test. It will make figuring out
>>>>>>>>>> JDK-8187143
>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8187143> a lot easier.
>>>>>>>>>>
>>>>>>>>>> Overall the test looks good. My main concern is the lack of
>>>>>>>>>> comments. It makes it hard for the reader to understand the
>>>>>>>>>> flow of the test and to understand some of the less obvious
>>>>>>>>>> actions being performed. That is especially true for this
>>>>>>>>>> test, which is doing some really bizarre stuff. Some of this
>>>>>>>>>> you cover in our RFR summary below, but that info really
>>>>>>>>>> needs to be in the test itself, along with additional
>>>>>>>>>> comments. For example, what does pauseAtDebugger() do? It
>>>>>>>>>> looks to me like it sets a breakpoint on the java script
>>>>>>>>>> debugger that has a class name that ends with ScriptRuntime
>>>>>>>>>> and the method name is DEBUGGER(). But I only figured that
>>>>>>>>>> out after staring at the code for a while, and recalling a
>>>>>>>>>> conversation we had a few weeks ago. It's also not described
>>>>>>>>>> why this is being done.
>>>>>>>>> I shall add more comments into the test code to make it easier
>>>>>>>>> to understand. However while I understand what is being done (
>>>>>>>>> e.g. adding breakpoint on Nashorn's internal DEBUGGER method)
>>>>>>>>> - unfortunately I too am not sure "why" it is being done. I do
>>>>>>>>> not have insight on what the author ( bug reporter) was trying
>>>>>>>>> to do..
>>>>>>>> That's ok. They "why" is because this is a test case
>>>>>>>> demonstrating a failure a user ran into. You might want to
>>>>>>>> mention that also, although the @bug reference might enough.
>>>>>>>
>>>>>>> Agreed as this is my understanding too.
>>>>>>>
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Here's another example:
>>>>>>>>>>
>>>>>>>>>> 126 while (!vmDisconnected) {
>>>>>>>>>> 127 try {
>>>>>>>>>> 128 Thread.sleep(100);
>>>>>>>>>> 129 } catch (InterruptedException ee) {
>>>>>>>>>> 130 }
>>>>>>>>>> 131 }
>>>>>>>>>>
>>>>>>>>>> I seem to also recall us discussing the need for this, but
>>>>>>>>>> can no longer recall the reason
>>>>>>>>> The above loop is there to make the debugger continue to run
>>>>>>>>> until it receives a VMdisconnect event either because the
>>>>>>>>> Debuggee crashed / got exception / finished.
>>>>>>>>> I shall add a comment for this as well.
>>>>>>>>>>
>>>>>>>>>> Another example is findScriptFrame(). What is the
>>>>>>>>>> significance of the frame whose class name starts with
>>>>>>>>>> jdk.nashorn.internal.scripts.Script$? I think I understand
>>>>>>>>>> (it's the generated java method for the javascript you setup
>>>>>>>>>> in ScriptDebuggee.doit()), but I can only figure this out
>>>>>>>>>> based on earlier conversations we had and your RFR comments
>>>>>>>>>> below. I'd expect the uninformed reader to spend a long time
>>>>>>>>>> coming the same conclusion.
>>>>>>>>> Again, I am not clear on the significance of popping frames
>>>>>>>>> until this method which is a generated java method for
>>>>>>>>> javascript in the debuggee. I could consult with the author
>>>>>>>>> and add those comments as well.
>>>>>>>> This is just to recreate the situation the customer saw when
>>>>>>>> running into the bug. We don't need to know the details of why
>>>>>>>> they were doing what they did, only that it resulted in a bug
>>>>>>>> being exposed. I'm mostly asking that you add comments that
>>>>>>>> explain what the test is doing, but not worry so much about
>>>>>>>> explaining the underlying reasons why the test was written in
>>>>>>>> the first place (although that might be useful as part of an
>>>>>>>> overall test summary comment at the top).
>>>>>>>
>>>>>>> Right.
>>>>>>> Thank you for the suggestion!
>>>>>>> I did not pay attention to it when pre-reviewed.
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The following are just a few minor things I noticed:
>>>>>>>>>>
>>>>>>>>>> Copyright should only have 2018.
>>>>>>>>>>
>>>>>>>>>> 57 } catch (Exception npe) {
>>>>>>>>>>
>>>>>>>>>> Probably best to call it "ex" instead of "npe".
>>>>>>>>>>
>>>>>>>>>> 85 NashornPopFrameTest bbcT = new
>>>>>>>>>> NashornPopFrameTest(args);
>>>>>>>>>>
>>>>>>>>>> It's unclear to me where the name "bbcT" comes from.
>>>>>>>>> I shall change that to npft or something like that.
>>>>>>>>>>
>>>>>>>>>> 134 if (failReason != null) {
>>>>>>>>>> 135 failure(failReason);
>>>>>>>>>> 136 }
>>>>>>>>>>
>>>>>>>>>> You have two classes that declare "String failReason" which
>>>>>>>>>> makes it a bit confusing to track which one the reader is
>>>>>>>>>> dealing with. Also, the NashornPopFrameTest version is
>>>>>>>>>> initialized to non-null, so doesn't that make the test always
>>>>>>>>>> fail when the above code is executed?
>>>>>>>>> Even though failReason is initialized, it is reset if the
>>>>>>>>> expected breakpoint is reached. And when the breakpoint is
>>>>>>>>> reached, it checks the Debuggee version of the field, if it is
>>>>>>>>> non null, then this field is set to the non null value - else
>>>>>>>>> it is set to null.
>>>>>>>>> I shall add some comments to make it less confusing.
>>>>>>>>>
>>>>>>>> So in the above check failReason has a double meaning (maybe
>>>>>>>> even triple). It could be set to its original value, which
>>>>>>>> means the breakpoint was never reached, or if the breakpoint is
>>>>>>>> reached it is set to ScriptDebuggee.failReason, which basically
>>>>>>>> represents the result of having called engine.eval(script). I
>>>>>>>> think it would be clearer if you just had a static flag to
>>>>>>>> indicate if the breakpoint was reached and just initialize
>>>>>>>> failReason to null.
>>>>>>>
>>>>>>> The static flag does not work as the debuggee is in a different
>>>>>>> VM process.
>>>>>> Of course. Rookie mistake on my part. :)
>>>>>
>>>>> I knew it but had done the same mistake. :)
>>>>>
>>>>>
>>>>>>>> Then the above becomes something like:
>>>>>>>>
>>>>>>>> if (breakpointReached) {
>>>>>>>> if (failReason != null) {
>>>>>>>> failure(failReason);
>>>>>>>> }
>>>>>>>> } else {
>>>>>>>> failure("Expected breakpoint in ScriptDebuggee:" +
>>>>>>>> ScriptDebuggee.BKPT_LINE + " was not reached");
>>>>>>>> }
>>>>>>>>
>>>>>>>> But then I wonder, why not just rethrow the exception when
>>>>>>>> engine.eval(script) fails and save yourself from having to
>>>>>>>> fetch ScriptDebuggee.failReason using the debugger, or is that
>>>>>>>> somehow part of what is being tested?
>>>>>>>
>>>>>>> It is not going to work if I understand things correctly.
>>>>>>> Please, check my comment above.
>>>>>>> In order to make it less confusing, I'd suggest to rename
>>>>>>> failReason to debuggeeFailReason on the debuggee side.
>>>>>> Yeah, maybe. But then you could also call it debuggeeFailReason
>>>>>> on the debugger side. That might make more sense. There's no
>>>>>> reason for ScriptDebuggee to add the "debuggee" prefix to one of
>>>>>> its own fields.
>>>>>
>>>>> Agreed.
>>>>>
>>>>>> I think there's still a need to have cleaner logic for indicating
>>>>>> if the breakpoint was reached. Right now we initialize failReason
>>>>>> to a potential failed reason string, then clear it if we hit the
>>>>>> break point and the debuggee had no previous errors. I think
>>>>>> using breakpointReached logic like I have above is a cleaner
>>>>>> approach.
>>>>>
>>>>> Got it, thanks.
>>>>> Yes, this will be more clear.
>>>> I shall change the logic as you have suggested and post another
>>>> patch for review.
>>>>
>>>> thanks,
>>>> Paru.
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Is there a reason why ScriptDebuggee doesn't just put
>>>>>>>>>> everything in main() and not have a doit() method?
>>>>>>>>> No there isn't a specific reason. I noticed that other tests
>>>>>>>>> were doing it - like BreakpointTest and for consistency and
>>>>>>>>> clarity, i followed that pattern.
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>> Paru.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>>
>>>>>>>>>> Chris
>>>>>>>>>>
>>>>>>>>>> On 2/7/18 12:25 PM, serguei.spitsyn@oracle.com
>>>>>>>>>> <mailto:serguei.spitsyn@oracle.com> wrote:
>>>>>>>>>>> Hi Paru,
>>>>>>>>>>>
>>>>>>>>>>> It looks good.
>>>>>>>>>>> Thank you a lot for taking care about this!
>>>>>>>>>>>
>>>>>>>>>>> Could we get at least one more review from the
>>>>>>>>>>> Serviceability team on this new test?
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Serguei
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2/2/18 09:35, Paru Somashekar wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Please review the fix for JDK-8193150.
>>>>>>>>>>>>
>>>>>>>>>>>> The fix introduces a new jtreg test, NashornPopFrameTest.
>>>>>>>>>>>> It is based on the original test from JDK-8187143
>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8187143> that was
>>>>>>>>>>>> provided by the customer.
>>>>>>>>>>>>
>>>>>>>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8193150
>>>>>>>>>>>> Webrev :
>>>>>>>>>>>> http://cr.openjdk.java.net/~psomashe/8193150/webrev/
>>>>>>>>>>>> <http://cr.openjdk.java.net/%7Epsomashe/8193150/webrev/>
>>>>>>>>>>>>
>>>>>>>>>>>> Here is a brief description of what the test does :-
>>>>>>>>>>>>
>>>>>>>>>>>> * The debuggee, creates and uses a Nashorn engine to
>>>>>>>>>>>> evaluate a simple script.
>>>>>>>>>>>>
>>>>>>>>>>>> * The debugger tries to set a breakpoint in Nashorn's
>>>>>>>>>>>> internal DEBUGGER method.
>>>>>>>>>>>> * When the breakpoint is reached, it looks for stack frame
>>>>>>>>>>>> whose method's declaring type name starts with (nashorn
>>>>>>>>>>>> dynamically generated classes)
>>>>>>>>>>>> "jdk.nashorn.internal.scripts.Script$".
>>>>>>>>>>>> * It then pops stack frames using the
>>>>>>>>>>>> ThreadReference.popFrames() call, up to and including the
>>>>>>>>>>>> above stackframe.
>>>>>>>>>>>> * The execution of the debuggee application is resumed
>>>>>>>>>>>> after the needed frames have been popped.
>>>>>>>>>>>>
>>>>>>>>>>>> This test is included in the ProblemList as it fails under
>>>>>>>>>>>> some circumstances (bug JDK-8187143)
>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8187143>. Is
>>>>>>>>>>>> always passes with the -Xint flag however always fails with
>>>>>>>>>>>> -Xcomp. It fails intermittently with the -Xmixed (default).
>>>>>>>>>>>>
>>>>>>>>>>>> thanks,
>>>>>>>>>>>> Paru.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic