Feature/DeadCodeEliminationExceptionsFix

#466 Declined
Repository
delors
Branch
develop
Author
  1. Jonathan Brachthäuser
Reviewers
Description

Generating code with CODE containing exception handlers such as

TRY(Symbol('FOO))
NOP
TRYEND(Symbol('FOO))
CATCH(Symbol('FOO), Some(ObjectType.Throwable))
ATHROW

leads to

Caused by: java.lang.IllegalArgumentException: 'try end 'FOO without try
[error]     at org.opalj.ba.CODE$.$anonfun$removeDeadCode$4(CODE.scala:287)
[error]     at org.opalj.ba.CODE$.removeDeadCode(CODE.scala:236)
[error]     at org.opalj.ba.CODE$.apply(CODE.scala:349)

if the try is identified as dead code.

This PR avoids this problem by restricting the extent of alive-marking from pseudocode to "pseudocode without exception instructions".

Comments (9)

  1. Patrick Müller

    This does not solve the problem for me totally, since the corresponding CATCH element is still present.

    InstructionElement(LabeledGOTO('b))
    TRY('try)
    InstructionElement(ICONST_0)
    TRYEND('try)
    LabelElement('b)
    CATCH('try,None)
    InstructionElement(ICONST_0)           
    

    becomes

    InstructionElement(LabeledGOTO(`b))
    LabelElement(`b)
    CATCH('try,None)
    InstructionElement(ICONST_0)
    

    after Deadcode elimination, which then causes the ExceptionHandlerGenerator to fail.

  2. Patrick Müller

    You have to replace line 148 with

    isLive(index) = false
    

    markedAsLive is not really relevant any more at the point where markAsDead is called. And could you please add a comment that the deadCode will not be removed correctly if a CATCH Element is located before its corresponding TRY?

  3. Patrick Müller

    Unfortunately this is still not sufficient. If the CATCH is between TRY and TRYEND and there is no live code between these (yes I've found code where this happens, and no its not from javac), we need to explicitly mark the tryend as dead in your added pass, too. Otherwise, building the Exception Handlers will fail since it only finds the single TRYEND. I guess the best way to do it is to track these in an additional map. For the Case that there is live code between TRY and TRYEND, and the CATCH is before the TRYEND, the whole TRY .. CATCH will not be removed, despite beeing dead, which is fine for me as long as building the Code itself succeeds. An example snippet for this is:

    LabeledGOTO('b),
    TRY('try),
    CATCH('try),
    ICONST_2,
    TRYEND('try),
    ICONST_1,
    'b,
    ICONST_0
    
  4. Patrick Müller

    This seems to be correct now. Thank you.

    Caveats are corner cases, i.e. any CATCH that is before TRYEND will lead to the non removal of otherwise dead code. And The JVM does not allow handler_pcs that are 0 (i've prepared code that adds a NOP before the Catch in such a case in order to fix this when this PR is merged).