Community
Participate
Working Groups
Eclipse compiler is generating dead bytecode when compiling the following method: private boolean test3(String strCode) { boolean value = false; try { if (!format(null, strCode)) { value = true; } switch(subLaunch(null)) { case OPTION0: this.exit(); break; case OPTION1: case OPTION2: value = true; case OPTION3: showError(getError(), true); break; } } catch(IOException e) { showException(e, "formatUrl"); value = false; } return value; } Here is what being generated. See GOTO L2 after IRETURN: private test2(Ljava/lang/String;)Z TRYCATCHBLOCK L0 L1 L1 java/io/IOException L0 ALOAD 0 ACONST_NULL ALOAD 1 INVOKESPECIAL asm/A.format (Ljava/lang/Object;Ljava/lang/String;)Z IFNE L2 ICONST_1 IRETURN >>> GOTO L2 L1 ASTORE 2 ALOAD 0 ALOAD 2 LDC "formatUrl" INVOKESPECIAL asm/A.showException (Ljava/io/IOException;Ljava/lang/String;)V ICONST_0 IRETURN L2 ALOAD 0 ACONST_NULL INVOKESPECIAL asm/A.subLaunch (Ljava/lang/Object;)I TABLESWITCH 0: L3 1: L4 2: L4 3: L5 default: L6 L3 ALOAD 0 INVOKESPECIAL asm/A.exit ()V GOTO L6 L4 ICONST_1 IRETURN L5 ALOAD 0 ALOAD 0 INVOKESPECIAL asm/A.getError ()Ljava/lang/Object; ICONST_1 INVOKESPECIAL asm/A.showError (Ljava/lang/Object;Z)V L6 ICONST_0 IRETURN MAXSTACK = 3 MAXLOCALS = 3
I don't think the source matches what you disassembled, but anyhow yes we may leave some dead code when performing branch chaining. This is a minor issue.
(In reply to comment #1) > I don't think the source matches what you disassembled, but anyhow yes we may > leave some dead code when performing branch chaining. > This is a minor issue. Sorry, I've posted a wrong snippet. Here is the right one: private boolean test2( String strCode) { try { if( !format( null, strCode)) { return ( true); } } catch( IOException e) { showException( e, "formatUrl"); return ( false); } switch( subLaunch( null)) { case OPTION0: this.exit(); break; case OPTION1: case OPTION2: return ( true); case OPTION3: showError( getError(), true); break; } return ( false); } I believe this issue is more serious then you think, because id adds an extra constraints to the bytecode analysis tools, especially those who have to run DFA (that by the way include recalculation of the StackMap/StackMapTable attributes for CDLC and Mustang's two pass verifiers). I am not saying that analysers should not handle this, but it is definetely increases the risk of DFA failures because it is an extremely rare case and more over Sun's compiler does not produce such anomaly (it is actually uses this GOTO as an island to jump from the IFNE instruction.
Simpler testcase: public class X { private void foo(boolean b) { try { if(b) { return; } } catch(Exception e) { } } } Bytecode at 5 is unreachable: // Method descriptor #15 (Z)V // Stack: 1, Locals: 3 private void foo(boolean b); 0 iload_1 [b] 1 ifeq 9 4 return 5 goto 9 8 astore_2 9 return Exception Table: [pc: 0, pc: 4] -> 8 when : java.lang.Exception [pc: 5, pc: 8] -> 8 when : java.lang.Exception
Created attachment 44157 [details] Proposed patch patch made against TARGET_321
Olivier - pls check the patch. also is it needed for stack table attribute ? we may want it for 3.2.1 then...
More similar scenarii (can replace return with throw or break/continue) public class X { boolean bool() { return true; } void foo() { try { if (bool()) { return; } } catch (Exception e) { } } int foo2() { try { while (bool()) { return 0; } } catch (Exception e) { } return 1; } long foo3() { try { do { if (true) return 0L; } while (bool()); } catch (Exception e) { } return 1L; } float foo4() { try { for (int i = 0; bool(); i++) { return 0.0F; } } catch (Exception e) { } return 1.0F; } double bar() { if (bool()) { if (bool()) return 0.0; } else { if (bool()) { throw new NullPointerException(); } } return 1.0; } void baz(int i) { if (bool()) { switch(i) { case 0 : return; default : break; } } else { bool(); } } }
Added TryStatementTest#test047-049
Fix consists in recording the position of the last abrupt completion in codestream (goto/athrow/?return) and during branch chaining notice that inlining occurred, and that last abrupt completion is at current position. If so, then chaining occurs, but no further (unreachable) goto is emitted.
(In reply to comment #5) > Olivier - pls check the patch. > also is it needed for stack table attribute ? we may want it for 3.2.1 then... I actually wonder what stack table is being generated for that dead instruction?. Either way code should be able to verify...
A stack map frame is needed after each goto/return/athrow/switch even if the code is unreachable. This is required to be able to do a linear walk of the bytecodes. We have a current bug that doesn't provide a stack map for some of these positions. I am working on it to get it fixed asap. The map at these location should reflect the state of the stack and the locals compare to the previous stack map entry.
(In reply to comment #10) > A stack map frame is needed after each goto/return/athrow/switch even if the > code is unreachable. This is required to be able to do a linear walk of the > bytecodes. > We have a current bug that doesn't provide a stack map for some of these > positions. I am working on it to get it fixed asap. > The map at these location should reflect the state of the stack and the locals > compare to the previous stack map entry. Right, but content of that stackmap also somehow dependend on the target where that goto pointing to. We had bit of a hassle to workaround this issue in ASM framework and had to workaround it and replace dead bytecodes. See more details at http://asm.objectweb.org/doc/developer-guide.html#deadcode
Eugene - do you have other instances of us producing dead code ? I will reattach a patch for TARGET_321, or can send you a JAR if you want for testing purpose. Beyond stack map issues, dead code is inaesthetic...
(In reply to comment #12) > Eugene - do you have other instances of us producing dead code ? > I will reattach a patch for TARGET_321, or can send you a JAR if you want > for testing purpose. > Beyond stack map issues, dead code is inaesthetic... I'd like to know about cases when Eclipse compiler can produce dead code, but I think we know how to handle them (basically we replace all dead branches with athrow + special stackmap table as Eric described in the document I referenced above).
Usually deadcode provides from semi-optimized bytecodes, based on branch chaining most likely like here. In the test case I attached in comment 6 (and in proposed patch as well) there are more instances of the same issue. All I am asking is that if you find more cases after using the patch, please tell us about it; even if you have protection against it.
Created attachment 44247 [details] Better patch
(In reply to comment #14) > All I am asking is that if you find more cases after using the patch, please > tell us about it; even if you have protection against it. Thanks Philippe. Will do.
Created attachment 44281 [details] Even better
Created attachment 44504 [details] Final patch Introduced notion of delegate branch label
(patch is made against TARGET_321 contents)
Attached patch for 3.2.1 (in case of crisis) Released for 3.3 M1 in HEAD
Created attachment 44549 [details] Final patch - take2 Fixed a typo which caused some bogus inlining (see TryStatementTest#test054)
Created attachment 44701 [details] Final patch - take3
Verified for 3.3 M1 using build I20060807-2000.
Please check bug 151756 if backported to 3.2.1.
New Gerrit change created: https://git.eclipse.org/r/127623
(In reply to Eclipse Genie from comment #25) > New Gerrit change created: https://git.eclipse.org/r/127623 SpotBugs found that. I see breakpoint is hit on few classes where delegate returns different values as this: CompletionEngine, ClassScope, SourceTypeBinding.
(In reply to Andrey Loskutov from comment #26) > (In reply to Eclipse Genie from comment #25) > > New Gerrit change created: https://git.eclipse.org/r/127623 > > SpotBugs found that. I see breakpoint is hit on few classes where delegate > returns different values as this: CompletionEngine, ClassScope, > SourceTypeBinding. OK, we know the change has some effect internally (I saw the same using SwitchTest.testSideEffect()), but did you ever observe a difference in the class files produced? Despite the different values internally, I could not find an example where observable behavior is affected. Strange enough, but makes it difficult to judge what effect the change has on users.
