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

[Jan Lahoda]

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.

Jan

> 
> 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
>