[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR JDK-8210760: [TEST] rewrite com/sun/jdi shell tests to java version - step4
From:       Chris Plummer <chris.plummer () oracle ! com>
Date:       2018-09-21 0:17:17
Message-ID: 94b6a847-3f53-6869-4af6-8386682f1e33 () oracle ! com
[Download RAW message or body]

On 9/20/18 4:11 PM, Alex Menkov wrote:
> Hi Chris,
>
> On 09/20/2018 15:07, Chris Plummer wrote:
>> Hi Alex,
>>
>> Can you put "expected statement printed by jdb" in a static final 
>> since it is used in 2 places? Otherwise it looks good. I don't need 
>> to see an updated webrev.
>
> Not sure how it can be done.
> The text has to be explicitly specified in the 1st line of 
> RedefineTTYLineNumberTarg.A method (as jdb prints the source).
Ah, of course. Looks fine as-is then.

Chris
>
> --alex
>
>>
>> thanks,
>>
>> Chris
>>
>> On 9/20/18 10:24 AM, Alex Menkov wrote:
>>> Looks like JDK-4660756 fixed line number, but didn't fix source.
>>> I filled JDK-8210927 for this.
>>>
>>> New webrev:
>>> http://cr.openjdk.java.net/~amenkov/sh2java/step4/webrev.02/
>>> changes (vs ver .01):
>>> - updated comment in StringConvertTest.java about arrays;
>>> - RedefineTTYLineNumber was updated to verify line numbers,
>>>    verification of the source at breakpoint is added, but commented 
>>> out (should be uncommented by fix for JDK-8210927)
>>>
>>> --alex
>>>
>>> On 09/14/2018 18:09, Chris Plummer wrote:
>>>> Hmm. I thought that's what the original bug was addressing.
>>>>
>>>>      27   * @summary TTY: Need to clear source cache after doing a 
>>>> redefine class
>>>>
>>>> Chris
>>>>
>>>> On 9/14/18 4:37 PM, Alex Menkov wrote:
>>>>> Looks like only line numbers are reported correctly, but the 
>>>>> content of the line content if not correct :)
>>>>>
>>>>> [jdb] Breakpoint hit: "thread=main", 
>>>>> RedefineTTYLineNumberTarg.A(), line=47 bci=0
>>>>> [jdb] 47                       System.out.println("in A, about to call B");
>>>>> [jdb]
>>>>> [jdb] main[1]
>>>>>
>>>>> [jdb] Breakpoint hit: "thread=main", 
>>>>> RedefineTTYLineNumberTarg.A(), line=46 bci=0
>>>>> [jdb] 46               public void A() {
>>>>> [jdb]
>>>>> [jdb] main[1]
>>>>>
>>>>> "public void A()" is a line 46 in the original file
>>>>>
>>>>> --alex
>>>>>
>>>>> On 09/14/2018 15:32, Chris Plummer wrote:
>>>>>> I think checking the output after the second breakpoint would be 
>>>>>> a good idea. However, rather than checking the line number, maybe 
>>>>>> just check the contents of the line, which should be included in 
>>>>>> the breakpoint event output.
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 9/14/18 3:23 PM, Alex Menkov wrote:
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> The file history does not contain any info about line number 
>>>>>>> dependency.
>>>>>>> I'll remove "11 before, 10 afterward" comment.
>>>>>>> Actually the test is not clear to me.
>>>>>>> Accordingly the test description jdb report obsolete line number 
>>>>>>> in the case, but the test does not verify its correctness, but 
>>>>>>> just checks _debuggee_ (not jdb) output for absence of "Internal 
>>>>>>> exception".
>>>>>>> The original bug is ancient, so it's hard to say if the test is 
>>>>>>> correct or not.
>>>>>>> I can add extra testing - extract reported line numbers (by 
>>>>>>> using regexp "line=(\d+)\b") and verify that 2st breakpoint is 
>>>>>>> reported with the expected line number (1 less than line of the 
>>>>>>> 1st breakpoint).
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> --alex
>>>>>>>
>>>>>>> On 09/14/2018 14:27, Chris Plummer wrote:
>>>>>>>> Hi Alex,
>>>>>>>>
>>>>>>>> Just one issue I see. For RedefineTTYLineNumber.java, the 
>>>>>>>> original test used to have this comment, which your removed:
>>>>>>>>
>>>>>>>>      52     // line number sensitive!!! Next line must be line 10.
>>>>>>>>
>>>>>>>> It's not clear to me why this test was ever line number 
>>>>>>>> sensitive, and whether you removed this sensitivity, or it just 
>>>>>>>> never existed. In any case, you left in the following comment, 
>>>>>>>> which maybe should also be removed:
>>>>>>>>
>>>>>>>>      47                 System.out.println("in A, about to call B"); // 
>>>>>>>> 11 before, 10 afterward
>>>>>>>>
>>>>>>>> Also, the println output from A() does not seem to match what 
>>>>>>>> the test is doing. There is no call to B():
>>>>>>>>
>>>>>>>>      46         public void A() {
>>>>>>>>      47                 System.out.println("in A, about to call B"); // 
>>>>>>>> 11 before, 10 afterward
>>>>>>>>      48                 System.out.println("out from B");
>>>>>>>>      49         }
>>>>>>>>
>>>>>>>> Maybe that's some bit rot. My understanding of the point of the 
>>>>>>>> test is while at the breakpoint at the start of A(), a redefine 
>>>>>>>> is done that deletes a line above this point, and jdi needs to 
>>>>>>>> make the appropriate adjustment of the current breakpoint line 
>>>>>>>> number. So calling B() does not play a roll in this, but 
>>>>>>>> perhaps it did a one point but the call was removed.
>>>>>>>>
>>>>>>>> Also, I don't see any indication of line number sensitivity 
>>>>>>>> here, but once again, maybe this is a bit rot issue and at one 
>>>>>>>> point it was line number sensitive.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> Chris
>>>>>>>>
>>>>>>>> On 9/14/18 12:59 PM, Alex Menkov wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> please review fix for
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8210760
>>>>>>>>> webrev:
>>>>>>>>> http://cr.openjdk.java.net/~amenkov/sh2java/step4/webrev.01/
>>>>>>>>>
>>>>>>>>> --alex
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>


[prev in list] [next in list] [prev in thread] [next in thread]