Bugzilla – Full Text Bug Listing
| Summary: | Error in JDT Core during AST creation | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Nicolas Lalevée <nicolas.lalevee> | ||||||
| Component: | Core | Assignee: | Srikanth Sankaran <srikanth_sankaran> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, jarthana, Olivier_Thomann, srikanth_sankaran | ||||||
| Version: | 3.7.1 | Flags: | daniel_megert:
pmc_approved+
Olivier_Thomann: review+ amj87.iitr: review+ |
||||||
| Target Milestone: | 3.6.2+J7 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Nicolas Lalevée
Created attachment 208520 [details]
TestClass.java
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);
}
}
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.
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).
Works alright with 3.6.2 and broken at 3.7. (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. (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. (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. 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. Created attachment 209189 [details]
Patch & tests - under test
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. 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. Ayush, please also look through the single line change. (In reply to comment #11) > Running all JDT/Core tests now. All tests are green BTW. 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. :) (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. +1. Code generation failures are serious problems. It can be difficult to find a workaround. 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. (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. Reopening for backport Released in 3.7 maintenance via commit b57994093099388baf36a2290f9fb387a20e2dc4 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 (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. Verified for 3.7.2 RC2 with build M20120118-0800 Verified for 3.8M5 using I20120122-2000 |