JDK-8208664: JDK8,JDK9-breakpoint on a continue doesnt work on jdb and intellij

[Vicente Romero]

On 9/5/19 1:19 PM, Jan Lahoda wrote:
> On 05. 09. 19 17:57, Vicente Romero wrote:
>> Hi Jan,
>>
>> The fix in the compiler looks good to me, although I'm not sure 
>> either what option should we expose this code with. Regarding the 
>> test, probably I'm missing something but I was expecting a simpler 
>> test generating a class file and then checking if the LNT was as 
>> expected.
>
> I was thinking of writing such a test (and maybe I still should add 
> it), but it felt insufficient. We have many tests that verify the 
> bytecode we generate by actually running the class (rather than 
> checking specific bytecodes generated). And this test is intended to 
> be similar: it actually runs the class under a debugger (jdb), sets a 
> breakpoint, and observes whether it stops appropriately.

right the test you wrote is great, but IMO it is more than needed in 
this case, it is complex, so maintaining it in the future could be more 
costly and also adds a dependency with an external tool which can change 
independently to javac.

>
> Jan

Vicente

>
>>
>> Thanks,
>> Vicente
>>
>> On 9/4/19 9:47 AM, Jan Lahoda wrote:
>>> Hi,
>>>
>>> There is a small issue in some cases when trying to put breakpoints 
>>> on break/continue (or stepping through code that has 
>>> break/continue). Consider this code:
>>> ---
>>>    static void test(boolean a, boolean b) {
>>>        for (int i = 0; i < 10; i++) {
>>>            if (a || b) {
>>>                continue; //set breakpoint here
>>>            }
>>>            System.out.println();
>>>        }
>>>    }
>>> ---
>>>
>>> This gets compiled into:
>>> ---
>>>          0: iconst_0
>>>          1: istore_2
>>>          2: iload_2
>>>          3: bipush        10
>>>          5: if_icmpge     28
>>>          8: iload_0
>>>          9: ifne          22
>>>         12: iload_1
>>>         13: ifeq          19
>>>         16: goto          22
>>>         19: invokestatic  #3                  // Method nop:()V
>>>         22: iinc          2, 1
>>>         25: goto          2
>>>         28: return
>>> ---
>>>
>>>
>>> The instruction that corresponds to the "continue" is the "goto" on 
>>> bytecode index 16. But, if "a" is true, that instruction is never 
>>> executed, because the test on bytecode index 9 will jump directly to 
>>> the "continue" target. We cannot generate LineNumberTable saying 
>>> that bytecode index 9 corresponds to the "continue", as it really 
>>> belongs to the if, and debuggers would in some cases stop on the 
>>> wrong line instead of stopping on the continue. So, the issue is 
>>> that if "a" is true, then there is never a real instruction executed 
>>> that corresponds to the "continue". What we can do is to change the 
>>> target if the "ifne" on bytecode index 9 to "16", so that the goto 
>>> is always executed, and everything would work OK in debuggers, 
>>> except that one additional jump is performed (and, in some rarer 
>>> cases, generated, as the actual "goto" can sometimes be skipped).
>>>
>>> I tried to compile the JDK with and without the proposed patch, and 
>>> about 1100 out of 31000 classfiles are changed, so this affects 
>>> roughly 3.5% of the JDK classfiles.
>>>
>>> The proposed patch is only enabling this behavior when 
>>> LineNumberTable is being generated, but that is fairly common, and 
>>> so it is not a big help. One of the alternatives would be to require 
>>> a special "-g" flag, like "-g:debuglines".
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8208664
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8208664/webrev.00/
>>>
>>> Any opinions?
>>>
>>> Thanks!
>>>
>>> Jan
>>