Community
Participate
Working Groups
On upgrading to Eclipse 3.6.0, I found erroneous reporting of "Dead code" in some of my projects. My unit tests revealed that the eclipse compiler was erroneously optimizing out a condition branch! I produced a minimal example, and retested under the current 3.6.1 M-build, M20100728-0800. Here is the code: ======================================================================= package bugdemo; import java.io.IOException; public class BugDemo { public static void main(String[] args) { String someVariable = null; int i = 0; try { while (true) { if (i == 0) { someVariable = "not null"; i++; } else { throw new IOException(); } } } catch (IOException e) { // having broken from loop, continue on } if (someVariable == null) { System.out.println("The compiler is buggy"); } else { System.out.println("The compiler is not buggy"); } } } ======================================================================= Compile and run under Eclipse 3.5 --> prints "The compiler is not buggy" Compile and run under Eclipse 3.6 --> prints "The compiler is buggy"
3.7 I20100805-1200 is also buggy
(In reply to comment #1) > 3.7 I20100805-1200 is also buggy Yes, I just checked it. This will be fixed for 3.7M2 and it will be backported to 3.6.1.
The compiler considers that someVariable is always null when hitting: if (someVariable == null). This is clearly wrong. Thanks for the test case. Ayushman, please investigate. This is a critical bug. Right now we report: [compiled 26 lines in 453 ms: 57.3 lines/s] [1 .class file generated] ---------- 1. WARNING in c:\tests_sources\X.java (at line 23) } else { System.out.println("The compiler is not buggy"); } ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Dead code ---------- 1 problem (1 warning) And this is wrong. Not detecting that someVariable can be different from null is causing the bug in the if statement.
(In reply to comment #3) > The compiler considers that someVariable is always null when hitting: > if (someVariable == null). > This is clearly wrong. > Thanks for the test case. > Ayushman, please investigate. This is a critical bug. It seems to me that this bug was always there but has now come to light because of our improved dead code detection and code optimization. Actually when we encounter while(true) loops, unless they have a break statement, we assume that the code after the loop will be unreachable and hence mark the loop's info as DEAD_END since it doesnt need to be utilised any further. Due to this the null changes inside the loop are lost when we exit the loop. This leads to the compiler checking null info of someVariable using the info we started out before going into the loop. Hence the warning. I checked 3.5 and the redundant null check warning is there as well. Its just that now we're optimizing based on that warning and hence it became more evident. Working on a patch.
Created attachment 176134 [details] proposed fix v1.0 + regression tests This patch makes sure that at the time of returning the final calculated flowInfos in an infinite while, for or do while loop, we dont return FlowInfo.DEAD_END, which doesnt have any null infos. Note that this works fine in case there's no try catch block surrounding the infinite loop without a break statement, since any code after that is dead code. But as the above example shows, for try catch we still would need the null infos from the actions inside the loop. The patch preserves null infos in case of try-catch block.
Srikanth, please review. TIA
The following test still fails: (passes with javac 5,6,7 and eclipse 3.5) import java.io.IOException; public class BugDemo { public static void main(String[] args) { String someVariable = null; int i = 0; try { while (true) { for (;;) { while (true) { if (i == 0) { someVariable = "not null"; i++; } else { throw new IOException(); } } } } } catch (IOException e) { // having broken from loop, continue on } if (someVariable == null) { System.out.println("The compiler is buggy"); } else { System.out.println("The compiler is not buggy"); } } }
(In reply to comment #4) > (In reply to comment #3) > > The compiler considers that someVariable is always null when hitting: > > if (someVariable == null). > > This is clearly wrong. > > Thanks for the test case. > > Ayushman, please investigate. This is a critical bug. > > It seems to me that this bug was always there but has now come to light because > of our improved dead code detection and code optimization. While 3.6 contained many welcome improvements around null analysis and dead code detection and the related diagnostics, in hindsight it is a questionable decision to have optimized the code generated on the basis of this analysis. It is one thing to emit a warning that is off by default, altogether quite another to generate code based on analysis that still has some kinks yet to be worked out. In the latter instance, we are just not on terra firma. As comment#4 alludes to, at 3.5 we do get the warning about dead code. I would not fix that warning in 3.6.x branch. I think the right course of action is to reverse the aggressive optimizations introduced by bug 293917. So my recommendation is to: - Reverse the optimization done in bug 293917 on both HEAD & 3.6.x - Raise a separate defect as needed for the wrong dead code message and track it only for HEAD.
(In reply to comment #8) > - Reverse the optimization done in bug 293917 on both HEAD & 3.6.x > - Raise a separate defect as needed for the wrong dead code message > and track it only for HEAD. Whilst I agree that fixing the code mis-generation is the most important thing, I strive to keep my projects completely warning-clean - therefore, I would be sad to see the spurious warnings dismissed as something not worth fixing in the 3.6.x stream. If a full fix is infeasible, would it be possible to simply disable any part of the dead code warning system which was proved to generate false positives, in 3.6.x?
Created attachment 176964 [details] proposed fix v2.0 + regression tests There are two parts to generating code for if-else statements: - The generation of the condition expression inside if(...) - The generation of then statements and else statements. The latter has always been linked to the UNREACHABLE bit being set or not. Bug 293917 however affected the generation of condition expression too if we know its a redundant null check. The problem occured even in this latter point because after the fix for bug 293917, there were lot more places where the UNREACHABLE was set because of which code gen optimization is happening in a lot more places than earlier. The setting of the bit itself is not wrong, but as we have seen there are corner cases where our null analysis is erroneous, and in some of them if we end up setting this bit, it ripples upto the generated code. There were two options basically to deal with this issue WITHOUT BREAKING THE NULL RELATED CHANGES MADE FOR 3.6 ( since we dont have any problem there really and we dont want to go back emitting false positives) : 1. Separate the setting of UNREACHABLE bit and optimization. I did not prefer this since it wouldve meant changing some behaviour we have since long. 2. Roll back fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=293917 i.e. dont set the UNREACHABLE bit when we know one branch is not going to be executed for sure. Again this approach would mean going back to false positives for null analysis, and reopening the 2 more bugs that had been fixed with the patch for bug 293917. So I thought a middle approach would be best and would also be minimal - setting another flag for the flowInfo for which the UNREACHABLE bit had been by static analysis after it figured out that it would never be executed ( due to some condition either always true/ always false) . This flag in the patch is FlowInfo.BYPASS_OPTIMIZE_CODEGEN. When the UNREACHABLE bit is set along with this flag, we report all null related and dead code warnings as usual. We dont however optimize code generation. This is all the above patch does, and I think this is the best approach for keeping the good things that went into 3.6 and removing the risky ones (i.e. aggressive code gen optimization).
(In reply to comment #10) > Created an attachment (id=176964) [details] > proposed fix v2.0 + regression tests Thanks for the detailed notes, Ayush. Let me study this in detail and see what the best course of action is.
Created attachment 177030 [details] Alternate patch This is an alternate patch that fixes both the wrong dead code warning and the incorrect code generation. This fix addresses only while loops with explicit throw statements inside (code similar to the one in comment# 0 and comment# 7 - both of which work ok with this patch) and needs to be generalized to other loops and exception throwing constructs (method invocation, object construction etc) I'll continue to work on it and as well as test it a good bit more.
(In reply to comment #10) > Created an attachment (id=176964) [details] > proposed fix v2.0 + regression tests > There were two options basically to deal with this issue WITHOUT BREAKING THE > NULL RELATED CHANGES MADE FOR 3.6 ( since we dont have any problem there really > and we dont want to go back emitting false positives) : [...] > So I thought a middle approach would be best and would also be minimal - In looking through your patch, it is evident that much water has flown under the bridge that setting the clock back (sorry for mixing up my metaphors) is not as simple an exercise as I had envisioned. The patch attached to comment#12 appears way simpler and indeed addresses the root cause of the problem (I'll type up an explanation of what is going on in the bug tomorrow - it is close to mid night now!) and at least ostensibly appears the right way to go. It needs to be polished up and after that we can take a call.
(In reply to comment #5) > Created an attachment (id=176134) [details] > proposed fix v1.0 + regression tests > > This patch makes sure that at the time of returning the final calculated > flowInfos in an infinite while, for or do while loop, we dont return > FlowInfo.DEAD_END, which doesnt have any null infos. I think it is worth recording from an academic point of view that with this (obsoleted) patch the following program begins to compile alright, while javac & eclipse 3.5, 3.6 reject this code as containing unreachable statements: import java.io.IOException; public class BugDemo { public static void main(String[] args) { String someVariable = null; int i = 0; try { while (true) { if (i == 0) { someVariable = "not null"; i++; } else { throw new IOException(); } } System.out.println("This is dead code, not complained about"); // <<---------------------------------------------------------------------------- } catch (IOException e) { // having broken from loop, continue on } if (someVariable == null) { System.out.println("The compiler is buggy"); } else { System.out.println("The compiler is not buggy"); } } }
Created attachment 177061 [details] Alternate patch with improvements This patch - Generalizes the fix to work with while, for, do-while loops. - Generalizes the fix to handle explicit throw statements as well as throws resulting out of method invocation, constructor call etc. - Adds several regression tests. Will continuing to test & polish the patch some more.
(In reply to comment #15) > Created an attachment (id=177061) [details] > Alternate patch with improvements Ayush, please begin reviewing and testing this patch. It will undergo some changes, but it wouldn't hurt to get started with familiarizing yourself with this patch.
Created attachment 177062 [details] Patch with more tests, comments ... Ayush, please review, test and verify this patch. If the tests you use differ sufficiently from what has been written by me, add them to NRT - Thanks!
All JDT/Core tests pass.
Created attachment 177157 [details] Proposed patch This is (hopefully) the final patch that incorporates some of the points Ayush and I came up with when discussing the patch attached to comment# 17 The fix is the same as the earlier one, but the current patch is - Significantly simpler, cleaner, shorter. - Eliminates needless code duplication. - Eliminates needless re-determination of exception catch blocks. - Adds more tests. Ayush, please give it a final once over, I would like to release it soon - TIA.
*** Bug 317829 has been marked as a duplicate of this bug. ***
(In reply to comment #19) > Created an attachment (id=177157) [details] > Proposed patch > > This is (hopefully) the final patch that incorporates Spoke too soon, (In reply to comment #20) > *** Bug 317829 has been marked as a duplicate of this bug. *** (1) This bug is surely a duplicate, but is ever so slightly different, The latest patch does not address it properly. (2) We also have a problem with: import java.io.IOException; public class BugDemo { public static void main(String[] args) { String someVariable = null; try { someVariable = "not null"; while (true) { throw new IOException(); } } catch (IOException e) { // having broken from loop, continue on } if (someVariable == null) { System.out.println("The compiler is buggy"); } else { System.out.println("The compiler is not buggy"); } } } Fortunately, the original fix valid, just needs a tweak.
Created attachment 177167 [details] Proposed patch - fixes all discussed issues & passes all tests.
Here is the (long overdue, long) explanation of what is going on here. Prior to code generation, the compiler performs code & data flow analysis to track things like initialization status of locals, definite/potential assignments to finals, reachability of code blocks, null status of variables etc. This is done to be able to issue diagnostics where mandated by the JLS (patently dead code, multiple assignments to finals, uninitialized locals usage) and also to support eclipse value additions (null pointer warnings, redundant checks etc) and to a limited extent optimize the code generated based on the analysis. Since a compiler cannot determine the set of paths that will be actually exercised in the program (an unsolvable problem), it performs conservative data flow analysis, computing and merging the data that flows into a point in program through all possible paths (pruning certain obvious cases) leading to that point. So if we have an if statement such as somevariable = null; if () { } else { } the "nullness" of the variable someVariable after the if is conservatively computed to be: if (neither if nor else touch it) nullness = definitely null; // unaltered. if (if sets to definite null && else sets it to definite null) nullness = definitely null; if (if sets it to definite nonnull && else also sets to definite non null) nulllness = definite nonnull; if (if sets it to definite nonnull && else sets to definite null) nullness = May be null; // merge black & white to get gray and so on. Thus the nullness state of a variable could be in one of many probable cases (definitely null, definitely nonnull, probably null, unknown ...) Proper analysis of loops is a bit trickier: Since a loop gets entered from the top as well as from the bottom by a loop back, we need to careful about using entry data flow while analysing the body of the loop. For example: someVariable = null; while () { if (someVariable == null) { // not necessarily a redundant null check } // some code here } Obviously, we cannot always warn about the someVariable == null check as being redundant since that variable could be altered down below in the loop. Proper analysis of loops requires two passes through its body, once from the top and once from the loop back path and a diagnostic could be issued only when it is valid for the first pass *and* the subsequent pass. However for performance reasons, by design, eclipse compiler performs only one pass through the loop body. So (a) to avoid spurious errors, we necessarily have to discard some information from the entry data flow information (not all, for example we have to throw any information we have about whether a variable is null or not, but we need not throw the information about whether a local has been initialized or not - once it is initialized it cannot become uninitialized, but from null, it can go to non null and vice versa) (b) to report errors, after the loop is traversed and the end of loop data flow is available, make deferred mode checks for errors within the body of the loop using the end of loop data flow state and entry into loop data flow state and report errors that are errors against both states. We can view (b) as a hidden quick pass over just the relevant portions of the body of the loop that have been carefully extracted from the first pass through the body of the loop. It was this hidden quick pass that was deficient and results in the current bug. As a result of this bug, the net effect is that it appears that the catch block can be entered only by a throw happening from the first iteration of the loop at which point someVariable is null. [It is obvious to a human reader that the first iteration can never throw the exception since i == 0 takes an alternate path. But the compiler doesn't attempt to evaluate that expression, so from a compiler's stand point, the first iteration could throw the exception] The fix is to allow the end of loop body data to flow back into the loop and thereby acknowledge that a throw can also happen from a "second or subsequent" iteration of the loop. The problem shows up only when the loop concerned is an infinite loop - because otherwise, the compiler allows the try blocks exit data to flow into the catch block (this is not totally accurate, but the whole analysis is conservative in nature and is so it is ok.)
Created attachment 177177 [details] Uptodate patch - More tests, - More precise determination of loop back data flow.
(In reply to comment #24) > Created an attachment (id=177177) [details] > Uptodate patch > > > - More tests, > - More precise determination of loop back data flow. The fix looks good.
Released in HEAD for 3.7 M2 in 3.6 maintenance stream for 3.6.1 As noted earlier, this fixes both the wrong warning and the bad code.
Verified for 3.6.1 RC2 using Build id: M20100825-0800
Verified for 3.7M2 using build I20100909-1700.
*** Bug 325940 has been marked as a duplicate of this bug. ***
I'm using Build id: 20130919-0819. The following snippet produces the same error: @Override public Object getDataValue(int columnIndex, int rowIndex) { Object retrievedObject = null; // this is what we will return int workingRowIndex = rowIndex; // zero-based, one-based, future buffering int workingColumnIndex = columnIndex; Row workingRow = this.rowList.get(workingRowIndex); ColumnDefinitions columnDefinitions = workingRow.getColumnDefinitions(); List<ColumnDefinitions.Definition> definitionsList = columnDefinitions.asList(); ColumnDefinitions.Definition definition = definitionsList.get(workingColumnIndex); DataType columnType = definition.getType(); String dataTypeNameString = columnType.toString(); if (null == workingRow) return "appropriate error message"; ... further code
(In reply to Charlie Kelly from comment #30) > I'm using Build id: 20130919-0819. > The following snippet produces the same error: Please open a fresh bug with a small but full snippet that can be used to reproduce. Thanks.
