[Soot-list] coffi bug, suggested fix inside..

[mbatch at cs.mcgill.ca]

It would appear there is a bug in Coffi when translating the heads of
catch blocks (due to Jimple's requirement of a IdentityStmt with a
caughtref in it). Specifically, if a unit is both part of normal control
flow _and_ the head for a catch block (which never happens in normal javac
bytecode but is nevertheless legal) then the normal control flow _might_
have a stack value overwritten by the caughtref. Take the following jasmin
example, where an exception is caught at label0, but control flow also
falls through:

.method public static main([Ljava/lang/String;)V
.throws java.lang.Exception
.catch java/lang/Exception from label2 to label1 using label0
    .limit stack 4
    .limit locals 1
    invokestatic java/lang/System/currentTimeMillis()J
    lconst_0
    lcmp
label2:
    ifgt label1
    new java/lang/Exception
    dup
    ldc "weird time"
    invokespecial java/lang/Exception/<init>(Ljava/lang/String;)V
    goto label0
label1:
    aload_0
    ifnull label3
    aconst_null
label0:
    athrow
label3:
    return
.end method


When this is run through Soot, the resulting Jimple code from label1 to
label3 looks like:

aload_0
ifnull label3
r4 := @caughtref
athrow

The "aconst_null" is removed because that value appears to never get used,
once the caughtref is inserted. This, of course, leads to improper stack
height and unverifiable code.

The only fix I can think of is to change the way caughtref IdentityStmts
are inserted into the Jimple in the coffi.CFG.jimplify method. Currently,
a new IdentityStmt is inserted before the normal handler unit, which
redirects all boxes pointing to the handler to the IdentityStmt (including
normal control flow).

I think all problems are fixed if, instead, the IdentityStmt is inserted
before the handler with _NO_ redirects. If the Stmt before the
IdentityStmt in the unit chain falls through then a new GotoStmt jumping
_around_ the IdentityStmt to the handler unit itself should be inserted. I
have tried this and it seems to work for my problem examples and also have
no bad side-effects when running against any other normal code. If nobody
has any objections I will check this change into the SVN:

The following is the current code in coffi.CFG:

units.insertBefore(newTarget, firstTargetStmt);
targetToHandler.put(firstTargetStmt, newTarget);


The new, fixed code, would look like this:

((PatchingChain)units).insertBeforeNoRedirect(newTarget, firstTargetStmt);
targetToHandler.put(firstTargetStmt, newTarget);
if (units.getFirst()!=newTarget) {
  Unit prev = (Unit)units.getPredOf(newTarget);
  if (prev != null && prev.fallsThrough()) {
    units.insertAfter(Jimple.v().newGotoStmt(firstTargetStmt), prev);
  }
}

Michael