Comment 1 Nicolas Lalevée 2011-12-17 18:02:05 EST

Created attachment 208520 [details]
TestClass.java

Comment 2 Srikanth Sankaran 2011-12-18 06:21:24 EST

Reproduced on 3.8 M4 and 3.7. Here is a smaller test case: 

public class X {
	void handle() {
		String s;
		label1: do {
			for (;;) {
				s = "";
				if (s == null) 
					continue label1;
			}
		} while (s != null);
	}
}

Comment 3 Olivier Thomann 2011-12-19 08:59:52 EST

For the last test case, the local variable ranges are wrong:
  // Method descriptor #6 ()V
  // Stack: 1, Locals: 2
  void handle();
     0  ldc <String ""> [15]
     2  astore_1 [s]
     3  aload_1 [s]
     4  ifnonnull 0
     7  aload_1
     8  ifnonnull 0
    11  return
      Line numbers:
        [pc: 0, line: 6]
        [pc: 3, line: 7]
        [pc: 7, line: 10]
        [pc: 11, line: 11]
      Local variable table:
        [pc: 0, pc: 12] local: this index: 0 type: X
        [pc: 3, pc: 7] local: s index: 1 type: java.lang.String
        [pc: 11, pc: 12] local: s index: 1 type: java.lang.String

s should be initialized from 3 to 12. There is no reason to have a range from 7 to 11 where it is not initialized.
This might come from the optimization to remove goto to the next line.
Investigating. I believe once the range is fixed, the stack map frames should be fine.

Comment 4 Olivier Thomann 2011-12-19 09:29:31 EST

I think it is coming from the fact that this code is called even if the condition is null.
if (this.preCondInitStateIndex != -1) {
codeStream.removeNotDefinitelyAssignedVariables(currentScope, this.preCondInitStateIndex);
}
This should not be called when there is no condition as there is no local to close the time to check the condition. See around line 322 in org.eclipse.jdt.internal.compiler.ast.ForStatement.generateCode(BlockScope, CodeStream).

Adding a null check fixed it. At least the local variable range is right and then the stack maps are properly generated.

The same issue exists with a while(true).
public class X {
    void handle() {
        String s;
        label1: do {
            while(true) {
                s = "";
                if (s == null) 
                    continue label1;
            }
        } while (s != null);
    }
}
In this case I think the local should be checked to call removeNotDefinitelyAssignedVariables only if the condition is equal to NotAConstant (not an optimized constant expression equals to true or false).

Comment 5 Srikanth Sankaran 2012-01-05 04:33:58 EST

Works alright with 3.6.2 and broken at 3.7.

Comment 6 Srikanth Sankaran 2012-01-05 23:21:06 EST

(In reply to comment #5)
> Works alright with 3.6.2 and broken at 3.7.

I think the ranges have always been suspect, but things work in 3.6.2
since the code generated is different, i.e we didn't generate code for
statements found to be dead by null analysis. So the code generated at
3.6.2 time  comes out to be:

 0: ldc           #15                 // String 
         2: astore_1      
         3: aload_1       
         4: ifnonnull     0
         7: goto          0
      LineNumberTable:
        line 6: 0
        line 7: 3
        line 5: 7
      LocalVariableTable:
        Start  Length  Slot  Name   Signature
               0      10     0  this   LX;
               3       4     1     s   Ljava/lang/String;
      StackMapTable: number_of_entries = 2
           frame_type = 0 /* same */
           frame_type = 6 /* same */

thereby side stepping the land mine.

Agree with Olivier's analysis in comment#4, patch will follow shortly.

Comment 7 Srikanth Sankaran 2012-01-06 01:49:27 EST

(In reply to comment #4)

> The same issue exists with a while(true).

[...]

> In this case I think the local should be checked to call
> removeNotDefinitelyAssignedVariables only if the condition is equal to
> NotAConstant (not an optimized constant expression equals to true or false).

More precisely, in both the cases, we should remove not definitely assigned
variables only if the condition is NOT known to be true.

Comment 8 Srikanth Sankaran 2012-01-06 05:04:08 EST

(In reply to comment #6)

> Agree with Olivier's analysis in comment#4, patch will follow shortly.

(In reply to comment #7)

> > In this case I think the local should be checked to call
> > removeNotDefinitelyAssignedVariables only if the condition is equal to
> > NotAConstant (not an optimized constant expression equals to true or false).
> 
> More precisely, in both the cases, we should remove not definitely assigned
> variables only if the condition is NOT known to be true.

Hmm. Analyzing further, I don't think this is the issue at all. See that
removing the not definitely assigned as described by the pre-condition
initialization state is _the_ orthodox way of doing things. Skipping these
removals when the condition is trivially determinable to be true at compile
time (as in for(;;) or while(true), for (;true;)) is at best an optimization.
In any case, post the body of inner loop, these assignments that happened
under the true condition would have be readded as per the merged state anyways.

This needs further investigation, but the real bug seems to be in
org.eclipse.jdt.internal.compiler.codegen.BranchLabel.place() where
some very old code mucks around with initialization ranges and messes
them up. (Search for see PR 1GIRQLA: ITPJCORE:ALL)

If we "optimize" the local removal based on the condition being true,
then the ranges are such that this code doesn't get triggered.

BTW, the original submitter test case fails with IAE all the way back
to 3.4.2 probably even earlier.

I'll continue to investigate this.

Comment 9 Olivier Thomann 2012-01-06 10:14:39 EST

Maybe I was not clear with my explanation. Since the condition is optimized (as it is always true), this creates a side-effect on the local initialization ranges.

Comment 10 Srikanth Sankaran 2012-01-09 04:14:14 EST

Created attachment 209189 [details]
Patch & tests - under test

Comment 11 Srikanth Sankaran 2012-01-09 04:20:11 EST

After chasing two plausible theories, I finally narrowed
it down to the root cause: The code that evaluates the 
condition at the bottom of the do-while loop is reached
via two pathways. One by free fall through the body of the 
loop and another via a continue in the body of the loop.

The initialization state of variables upon reaching the
condition stage of the loop then is the merger of the
two possible arcs - we were not taking this into consideration 
and treating it as though the body of the loop is the only way
to reach the end of the loop. A one line fixes the problem and 
both comment#0 test case and the much simplified comment#2 test
case now pass with this patch.

Running all JDT/Core tests now.

Comment 12 Srikanth Sankaran 2012-01-09 06:31:48 EST

Olivier, please take a look, it is a single line change. I am considering this
for 3.7.2 even though it is not a regression.

Comment 13 Srikanth Sankaran 2012-01-09 06:32:26 EST

Ayush, please also look through the single line change.

Comment 14 Srikanth Sankaran 2012-01-09 06:57:36 EST

(In reply to comment #11)

> Running all JDT/Core tests now.

All tests are green BTW.

Comment 15 Ayushman Jain 2012-01-09 08:14:03 EST

Fix looks good and completes the fix for bug 279183.

Looks safe for backporting as well, though not sure whether we should play with initializations in the RC week. :)

Comment 16 Olivier Thomann 2012-01-09 09:50:12 EST

(In reply to comment #12)
> Olivier, please take a look, it is a single line change. I am considering this
> for 3.7.2 even though it is not a regression.
Looks good to me.

Comment 17 Olivier Thomann 2012-01-09 09:51:14 EST

+1. Code generation failures are serious problems. It can be difficult to find a workaround.

Comment 18 Srikanth Sankaran 2012-01-09 09:59:34 EST

Thanks Olivier and Ayush. I have released it for 3.8 M5 via
http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=93c94a7685512a759bb72181caf791ff94ebd920.

Dani, do you want this for 3.7.2 ? On the one hand this problem has
existed for a long time, at least since 3.4.2 perhaps even earlier.

On the other, it is bad code generation problem that can be difficult
to work around. The fix looks safe, just a one line change and has been
reviewed by Olivier and Ayush.

Comment 19 Dani Megert 2012-01-09 10:02:23 EST

(In reply to comment #18)
> Thanks Olivier and Ayush. I have released it for 3.8 M5 via
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=93c94a7685512a759bb72181caf791ff94ebd920.
> 
> Dani, do you want this for 3.7.2 ? On the one hand this problem has
> existed for a long time, at least since 3.4.2 perhaps even earlier.
> 
> On the other, it is bad code generation problem that can be difficult
> to work around. The fix looks safe, just a one line change and has been
> reviewed by Olivier and Ayush.

+1 for backport.

Comment 20 Ayushman Jain 2012-01-10 00:36:23 EST

Reopening for backport

Comment 21 Ayushman Jain 2012-01-10 00:38:49 EST

Released in 3.7 maintenance via commit b57994093099388baf36a2290f9fb387a20e2dc4

Comment 22 Ayushman Jain 2012-01-10 01:47:11 EST

Released in 3.6.2+Java7 branch via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=R3_6_maintenance_Java7&id=9a77c72a2cb563b0ee2bb28ee4096f7d7e5e97f2

Comment 23 Dani Megert 2012-01-10 02:47:48 EST

(In reply to comment #22)
> Released in 3.6.2+Java7 branch via
> http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?h=R3_6_maintenance_Java7&id=9a77c72a2cb563b0ee2bb28ee4096f7d7e5e97f2

Please not that you still need to do the build input manually (as in contrast to 3.7.2), see bug 364676.

Comment 24 Jay Arthanareeswaran 2012-01-19 02:29:56 EST

Verified for 3.7.2 RC2 with build M20120118-0800

Comment 25 Jay Arthanareeswaran 2012-01-24 00:02:29 EST

Verified for 3.8M5 using I20120122-2000