Community
Participate
Working Groups
Build Identifier: Helios Service Release 1, Build id: 20100917-0705 Compile the source below in 'Helios Service Release 1'. The if statement never gets executed. When looking at the byte code you could see that the if statement was eliminated. public class Main { public static void main( String ...strings ) { Integer i1 = null; Integer i2 = (i1 = getInt()) == null ? null : i1; if( i2 != null ) { /* this never gets executed in eclipse 3.6.1 * its flagged as dead code * the if statement does not exist in the byte code compiled with 3.6.1 * it is present in 3.5 byte code and does get executed */ System.out.println("Shut down nuclear reactors now!!"); } } private static Integer getInt() { return new Integer(0); } } Reproducible: Always Steps to Reproduce: 1. Compile code 2. Run code 3. Ensure the airplane you will be flying in on you next trip does not use any code compiled with the JDT compiler.
Reproduced with HEAD. Ayushman, please investigate.
Replacing the two lines with: Integer i1 = getInt(); Integer i2 = i1 == null ? null : i1; makes the code work again. The problem comes from assignment inside the condition of a conditional expression.
Created attachment 188459 [details] Add regression tests Add two regression tests. One with assignment inside the condition expression condition and one with the assignment inside the local declaration. The second one already works. I still want to add it to make sure both cases are covered.
This is a regression over 3.5.2. So this needs a fix for Indigo.
This looks like a nasty manifestation of bug 324178, which doesn't have an easy fix. Will need to investigate further.
(In reply to comment #5) > This looks like a nasty manifestation of bug 324178, which doesn't have an easy fix. I don't think this is exactly the same issue. In this case, the problem comes from the fact that the null info of the local declaration is retrieved before the initialization of the local is done. Let me know if I missed something.
(In reply to comment #6) > (In reply to comment #5) > > This looks like a nasty manifestation of bug 324178, which doesn't have an easy fix. > I don't think this is exactly the same issue. In this case, the problem comes > from the fact that the null info of the local declaration is retrieved before > the initialization of the local is done. > Let me know if I missed something. The problem in this case that when we call the nullStatus() method from LocalDeclaration.analyseCode while trying to analyse the declaration of i2, we havent yet initialized the condition of the conditional expression. That means, we havent really analysed i1 = getInt() yet, and at this stage we're still considering the old value of i1 ie. null. So null status from both branches of the conditional expression comes out to be null. The analysis of the condition is done together with the analysis of the whole conditional expression after the nullStatus is calculated. This is done in this.initialization.analyseCode(currentScope, flowContext, flowInfo) .unconditionalInits(); on line 76 of LocalDeclaration.analyseCode() We should thus make sure that the condition of the conditional expression gets analysed before we calculate the null status. I'm testing a possible fix.
It tried to move the nullStatus call after the analysis of the initialization and it seems to fix this issue.
(In reply to comment #8) > It tried to move the nullStatus call after the analysis of the initialization > and it seems to fix this issue. Yeah, thats exactly what I was running the test suite on before going to sleep last night. Woke up to find all tests green. :)
Created attachment 188496 [details] proposed fix v1.0 + regression tests This patch fixes both the case when ConditionalExpression is used in LocalDeclaration as well as Assignment by making sure nullStatus is calculated only after analysis of the initialization is done
Released in HEAD for 3.7M6. The fix should be available in tomorrow's I build. Harry, hope you have a pleasant flight aboard JDT airways. ;)
(In reply to comment #5) > This looks like a nasty manifestation of bug 324178, which doesn't have an > easy fix. Bug 324178 is not present in 3.6.x, so it can't be a manifestation of that one. The bug is pretty bad and from a bird's eye view the fix looks good. Suggest to fix for 3.6.2 but final word is with Srikanth and Olivier.
Ayush, I would have liked to see some post-mortem in terms of when exactly this bug got introduced and in what context. Please share your findings here. I'll study the patch and experiment with the fix a bit.
Since the runtime behavior is impacted, I vote for a 3.6.2 inclusion.
(In reply to comment #13) > Ayush, I would have liked to see some post-mortem in terms of > when exactly this bug got introduced and in what context. > Please share your findings here. The first thing to note is that the behaviour was wrong even prior to 3.6. The only difference is that we reported the redundant null check warning but did not report the dead code, and hence did not optimize it out. The fix for bug 302446 improved deadcode detection because of which we started reporting deadcode at all places where we should according to our analysis. Due to this, what was a simple redundant null check warning in 3.5 turned into a null check warning + dead code warning. Now dead code warning is given via org.eclipse.jdt.internal.compiler.ast.Statement.complainIfUnreachable(FlowInfo, BlockScope, int), which sets the ASTNode.IsReachable bit on the statement. (the then block in this case). Thus, during code generation we dont generate code for this statement. Bottomline is, the problem itself had been there all along, but due to improved deadcode detection, has manifested itself in an ugly way.
(In reply to comment #15) >which "sets" the ASTNode.IsReachable bit on the statement. i meant RESET!
(In reply to comment #13) > I'll study the patch and experiment with the fix a bit. It looks good to me. I second the decision to back port this to 3.6.2. Ayush, please prepare a 3.6.2 patch. We need to run all the tests on the branch.
Created attachment 188524 [details] path for 3.6.2 Srikanth, please give this patch a final once over and i'll release it. Thanks
Released in R3_6_maintenance for 3.6.2.
+1 for backporting to 3.6.2
I'm wondering if anyone already knows and can net this out for me. We in WTP were using R36_RC4 base builder that included the "old" compiler (jdt.core) versioned 3.6.0.v_A58. The newest base builder (in branch) tagged with r36x_v20110209 contains the compiler (jdt.core) of version 3.6.2.v_A76_R36x. So, a) sounds like other fixes besides fixing this regression are in the difference from v_A58 and v_A76_R36x. From what I can tell, from bugzilla query, there's 14 fixes targeted to 3.6.1 or 3.6.2. Any reasons some of these are really important to "pick up" or "risky to pick up" (for us in WTP). The bugzilla query will be ugly, but if pastes correctly, would be https://bugs.eclipse.org/bugs/buglist.cgi?query_format=advanced;short_desc=compiler;bug_status=RESOLVED;bug_status=VERIFIED;bug_status=CLOSED;short_desc_type=allwordssubstr;target_milestone=3.6.1;target_milestone=3.6.2;product=JDT;classification=Eclipse More importantly, b) was the "regression' even in the compiler even in version v_A58? That is, was v_A58 essentially correct, and safe to stick with? I ask, because in WTP, I tried the new compiler (via the new base builder) and the p2 comparator found 7 classes changed, as I am tracking in bug 336780. So, I'm just trying to figure out (easily) if it is better to stick with old compiler (and avoid (small) risk of changed byte codes), or better to get current. My _guess_ is the changes in our bytes codes would not be especially significant (probably just dead code left out) ... but thought I'd ask here if anyone had an educated opinion? (rather than just me guessing :) Much thanks,
> More importantly, b) was the "regression' even in the compiler even in version > v_A58? That is, was v_A58 essentially correct, and safe to stick with? The bug was in 3.6 already. You would only be safe if you know for sure that you don't have the code pattern from comment 0 in your code. > I ask, because in WTP, I tried the new compiler (via the new base builder) and > the p2 comparator found 7 classes changed, as I am tracking in bug 336780. So, > I'm just trying to figure out (easily) if it is better to stick with old > compiler (and avoid (small) risk of changed byte codes), or better to get > current. Use the new one. It also fixes three other bugs that caused wrong byte code to be generated among which bug 320414 is serious.
Verified for 3.6.2 using build M20110209-1607.
We are also updating to the new compiler / basebuilder in the TM project (tracked by bug 337045). Since we haven't been using the comparator in the TM project so far, I cannot tell if I need to re-tag any bundles. I would really appreciate somebody to help out and check the previous build against the new one - also to verify bytecode correctness. For details see bug 337045 comment 2. Note that I found the new basebuilder to potentially have an issue with computing the feature qualifier of including features (multiple nesting). This might be a regression. Tracked by bug 337053.
(In reply to comment #24) > We are also updating to the new compiler / basebuilder in the TM project > (tracked by bug 337045). Since we haven't been using the comparator in the TM > project so far, I cannot tell if I need to re-tag any bundles. I would really > appreciate somebody to help out and check the previous build against the new > one - also to verify bytecode correctness. I'll check the two repos that you provided in the other bug report.
(In reply to comment #25) > I'll check the two repos that you provided in the other bug report. Thanks Olivier. I'll wait with declaring TM 3.2.2rc4 until I get your feedback.
Thanks for fixing this so quick. It was a pretty nasty bug to find because the code in question was machine generated and buried in 500k LOC. Luckily I don't know any human that would write code like that.
Martin, it will take some time to verify everything. I'll try to finish it tomorrow.
(In reply to comment #26) > Thanks Olivier. I'll wait with declaring TM 3.2.2rc4 until I get your feedback. Martin, the source code for the DataStore class is different. The bytecodes for this file is therefore different. Everything else looks the same. I don't know if this is expected.
(In reply to comment #29) > (In reply to comment #26) > > Thanks Olivier. I'll wait with declaring TM 3.2.2rc4 until I get your feedback. > Martin, the source code for the DataStore class is different. The bytecodes I am referring to the class: org/eclipse/dstore/core/model/DataStore.class --- in org.eclipse.dstore.core....jar I also need to check two other classes in details. org/eclipse/rse/services/clientserver/archiveutils/SystemTarHandler.class org/eclipse/rse/services/clientserver/archiveutils/SystemZipHandler.class Results will follow shortly.
(In reply to comment #30) > I also need to check two other classes in details. > org/eclipse/rse/services/clientserver/archiveutils/SystemTarHandler.class > org/eclipse/rse/services/clientserver/archiveutils/SystemZipHandler.class > Results will follow shortly. I don't see anything wrong with the changes in these two files. There is a removed null check, but I don't see how the variable can be null at this location. Don't you have a redundant null check warning in these two files ?
Thanks for checking, Olivier! In fact the change in the DataStore class is expected, it is our one and only change between TM 3.2.2RC2 and TM 3.2.2RC4. Sorry for not mentioning this expected diff (on the other hand it's a good sanity check for the checks, to see you have found this diff :) The o.e.dstore.core bundle has been re-tagged for the expected change: http://dev.eclipse.org/mhonarc/lists/tm-cvs-commit/msg00142.html http://dev.eclipse.org/mhonarc/lists/tm-cvs-commit/msg00147.html For SystemTarHandler and SystemZipHandler it looks like I _should_ usually also re-tag o.e.rse.services which includes clientserver.jar -- since the Comparator would show a diff. On the other hand, the change is trivial so I might also just go and rename the existing M-build you checked into RC4, and avoid disruption to the Helios SR2 train, correct?
(In reply to comment #32) > For SystemTarHandler and SystemZipHandler it looks like I _should_ usually also > re-tag o.e.rse.services which includes clientserver.jar -- since the Comparator > would show a diff. On the other hand, the change is trivial so I might also > just go and rename the existing M-build you checked into RC4, and avoid > disruption to the Helios SR2 train, correct? yes, both versions are correct. So only the DataStore has to be retagged as it contains an expected change. o.e.rse.services could be tagged for Indigo.
