Community
Participate
Working Groups
Fup of bug 185682 as observed in bug 185682 comment 78: Some local variables that are flagged as "The value of the local variable foo is not used" are still generated even if -preserveAllLocals is not set. Test case from bug 185682 comment 78: import java.util.List; import java.util.ArrayList; import java.util.Iterator; public class X { public static void main(String[] args) { List l = new ArrayList(); int i = 0; for (Iterator iter = l.iterator(); iter.hasNext(); i++) { System.out.println(iter.next()); } } } Actually, all new test cases from bug 185682 are affected. The problem is described in bug 185682 comment 79: "... we seem to be stuck here, because we only know *after* generateStatements which locals are actually used. Not generating purely "compound used" variables is asking for trouble, if it turns out that the value of "i++" is actually used. Right now I don't see an easy way out, so I will leave at the previous state, i.e., locals with useFlag < 0 will be generated. Rambling about possible solutions I could think of: - restarting generateCode if unused state of a local was detected late, but I don't think the infrastructure for such restart is in place - change signatures of all analyzeCode method adding a flag valueRequired just as we have in generateCode -> need to change all AST classes below Expression (Statement?)." Setting severity to minor, because the warning is given to the user who may (should) remove the unused local manually. Also, I'm not really sure this issue is worth the (expected) effort. Still documenting here for completeness, since this is indeed an inconsistency.
> Setting severity to minor, because the warning is given to the user who > may (should) remove the unused local manually. Also, I'm not really sure > this issue is worth the (expected) effort. Still documenting here for > completeness, since this is indeed an inconsistency. I agree on this given the default is to preserve the unused local variables.
Created attachment 181676 [details] First draft Untested patch.
The patch doesn't work. .. int i = 0; return i++; leads to a verify error. I am investigating.
Created attachment 181728 [details] Updated patch
(In reply to comment #4) > Created an attachment (id=181728) [details] > Updated patch Oh, you do have some restart infrastructure in place (for RESTART_IN_WIDE_MODE). That's cool! One more comment from looking at the patch: what are you saying by localBinding.useFlag = Integer.MAX_VALUE; ? If the value needs to be different from USED I suggest adding (and documenting) another constant in LocalVariableBinding, no?
(In reply to comment #5) > what are you saying by > localBinding.useFlag = Integer.MAX_VALUE; > ? I just wanted to put a value high enough so that doing useFlag-- will keep it high than USED. > If the value needs to be different from USED I suggest adding (and > documenting) another constant in LocalVariableBinding, no? Clearly. This is just a draft to see what needs to be done to fix this issue. It requires some polishing.
(In reply to comment #6) > (In reply to comment #5) > > what are you saying by > > localBinding.useFlag = Integer.MAX_VALUE; > > ? > I just wanted to put a value high enough so that doing useFlag-- will keep it > high than USED. We do useFlag-- only if useFlag <= UNUSED. Have you tried USED instead of MAX_VALUE?
Using USED does the trick as well. I'll add some regression tests and continue to polish it after M3 is declared.
Created attachment 181737 [details] Updated patch New update with some regression tests.
Restarting is required when no resolved position has been assigned to a local variable and it needs to be generated (valueRequired = true). So in this case, the local is tagged as used and we restart the code generation. It seems to work fine. Will review the patch and do more testing after M3.
Created attachment 182485 [details] Proposed fix + regression tests
Released for 3.7M4. Ayushman, please review. Regression tests: org.eclipse.jdt.core.tests.compiler.regression.ProgrammingProblemsTest#test0052 org.eclipse.jdt.core.tests.compiler.regression.ProgrammingProblemsTest#test0053 org.eclipse.jdt.core.tests.compiler.regression.ProgrammingProblemsTest#test0054
I reverted it as it broke some JDT/UI tests.
Created attachment 182577 [details] improved patch This is what I'd suggest to avoid the regression reported in bug 329613. The story is: when preserveAllLocalVariables is set, a useFlag < 0 was set to USED, thus erasing the information about 'compound' access. BlockScope.computeLocalVariablePositions only has to ensure that useFlag != UNUSED after the given branch. I also included the example from bug 329613 as a new JDT/Core test.
One more note on assumptions regarding the useFlag: Alerted by bug 329613 I searched all uses that might assume useFlag >= 0 and found two more suspicious locations: methods manageEnclosingInstanceAccessIfNecessary in SingleNameReference and QualifiedNameReference. Both contain a switch: switch(localVariableBinding.useFlag) { case LocalVariableBinding.FAKE_USED : case LocalVariableBinding.USED : currentScope.emulateOuterAccess(localVariableBinding); } which doesn't handle the < 0 case. In order to enter this branch in SingleNameReference with useFlag < 0 we'd need (a) a local variable accessed from a local class of the current method Only so we trigger outer emulation for a local variable (b) this access must be a 'compound' access (compound assign/postIncr) Only so we get useFlag < 0 No legal example can be created from this, since (a) requires the local to be final and (b) requires it to be non-final. For QualifiedNameReference consider this example: class X { private int i=1; int bar() { final X x = new X(); class Local { int foo() { --x.i; return 3; } }; return new Local().foo(); } } i is correctly flagged, whereas x is considered as USED, i.e., we can't create a case where x's useFlag < 0. This means the current implementation is safe. One *might* briefly think that the value of x isn't actually used but since x.i could cause an NPE we should never report this as unused. Besides: after removing the unused i, then x will be flagged anyway. Makes sense?
(In reply to comment #15) > Makes sense? Yes, thanks for the analysis, Stephan. I ended up with the same patch during the week-end. If the value is not UNUSED, we can simply keep it and if it is UNUSED, then we can set it as USED. I'll release the patch later today. Thanks again.
(In reply to comment #16) > I'll release the patch later today. > Thanks again. I found new issues with this patch. Needs more investigation. I'll provide more regression tests soon to address these new problems.
One of the pb can occur if more than one variable requires a restart because its value is required but it has no resolved position. In this case, we need to "loop" through the code generation till all variables have been properly generated. Code like this shows the pb: public class X { static int foo() { int i = 2; int j = 3; return (i += j *= 3); } public static void main(String[] args) { System.out.println(foo()); } } I have a fix for this issue. I am investigating a second issue with secret local.
Created attachment 182648 [details] Proposed fix + regression tests I forgot to reset the local variables inside the code stream so secret locals were not injected back into the code stream in 1.6 mode when the code generation was restarted leading to internal compiler error.
I'll rerun all tests before releasing it.
Created attachment 182652 [details] Proposed fix + regression tests Same patch with more regression tests inside EnumTest (clinit and constructor restart tests).
Created attachment 182714 [details] Proposed fix + regression tests Last patch. When the constant of the expression is not a constant, then we cannot skip the value generation. In case of compound assignment, I leave the code generation as it was done before. I'll see if there is a way to generate just the value of the constant and not the actual variable loading when no value is required.
Created attachment 182763 [details] Proposed fix + regression tests Latest patch
Fixed and released for 3.7M4.
Verified for 3.7M4 using build I20101206-1800.
While analyzing bug 333678, I did find one case where it appears this is not a complete fix. I'm not at all worried about it myself, but if it helps you, I can open a new bug with full details. Essentially, its part of an anonymous inner class and snippet looks like this: int width = area.width - 2 * table.getBorderWidth() * 2; if (preferredSize.y > area.height + table.getHeaderHeight()) { // Subtract the scrollbar width from the total column width // if a vertical scrollbar will be required Point vBarSize = table.getVerticalBar().getSize(); width -= vBarSize.x; } It is the variable 'width' that is not used; at beginning of snippet, and later at end of if clause. In the IDE, the first occurrence of 'width', where it is declared, now (when using 37 M4) is flagged as unused, but according to the decompiler code, it is not completely removed (though, byte codes are slightly different that with 3.6 compiler. The (JAD) decompiler for 3.6 byte codes produced some pseudo code like this: /* 259*/ int width = area.width - 2 * table.getBorderWidth() * 2; /* 260*/ if(preferredSize.y > area.height + table.getHeaderHeight()) { /* 263*/ Point vBarSize = table.getVerticalBar().getSize(); /* 264*/ width -= vBarSize.x; } The decompiled version for 3.7 looks like this: /* 259*/ int _tmp = area.width; /* 259*/ table.getBorderWidth(); /* 260*/ if(preferredSize.y > area.height + table.getHeaderHeight()) { /* 263*/ Point vBarSize = table.getVerticalBar().getSize(); /* 264*/ int _tmp1 = vBarSize.x; } Notice "width" (now called _tmp by decompiler, has been separated from table.getBorderWidth();, as if getting read to remove _tmp .... but, apparently it is left in ... in that spot, as well as the one at end of if clause. Like I said ... I myself am not the least bit worried that the compiler is "wrong" (if it is) ... and the code is pretty sloppy and maybe incorrect in any case ... but if this case surprises any of you, I can open new bug and provide more detail (such as whole source, bundle, compiled jars) ... if it helps you.
(In reply to comment #26) > While analyzing bug 333678, I did find one case where it appears this is not a > complete fix. Looking at the actual bytecodes (not the re-engineered pseudo code from JAD) I don't see any traces of the variable 'width' but only those instructions remain that could possibly cause a side effect (an NPE in area.width or an arbitrary side effect in getBorderWidth()). Apparently the names _tmp etc. result from JAD's attempts to express the remaining instructions as valid Java source. E.g., area.width; wouldn't be a legal Java statement, but that's exactly what remains in the byte code. So, from my analysis everything works as intended here.
