Re: remove-unreachable-traps

[John Jorgensen <jorgnsn@lcd.uregina.ca>]

>>>>> "olhotak" == Ondrej LHOTAK <olhotak@sable.mcgill.ca> writes:

    olhotak> I was wondering whether anyone knows why the
    olhotak> remove-unreachable-traps option defaults to false in
    olhotak> the UnreachableCodeEliminator. In fact, I wonder why
    olhotak> it's even an option to not remove.

I'm the guilty party, but it was a year ago and I'm not sure I
remember all of my motivation.  (I also suspect it's not the
real reason for the problem you've observed, but that comes at
the end of this message.)

The core issue is that with the addition of ExceptionalUnitGraph,
it becomes possible for a handler to become unreachable for a new
reason.  Before---when we assumed that all trapped units had the
potential to throw an exception that the trap would catch---the
only way for a trap to become unreachable was if all of the
trapped units were optimized away.  In this case, I agree that
there's no reason not to remove the handler as well.

With the addition of ExceptionalUnitGraph, it becomes possible
for a handler to become unreachable because the ThrowAnalysis
says that none of the trapped units can throw the exception that
the trap catches.  If the exception analysis is correct, the
semantics of the program will not be changed if you remove the
trap, but I was hesitant about removing it for two, admittedly
dubious, reasons.

One was simple diffidence, if a programmer writes 

  try {
    j = i + j * k;
  } catch (ArithmeticException e) {
    // ... whatever ...
  }

I have a nagging sense that he or she might have some reason for
including the catch even though the statement can't throw an
ArithmeticException.  I admit I can't think of what the reason
could be, but just because I can't think of it, that doesn't mean
that none exists. So I didn't want to make it impossible for
somebody to retain the unreachable handlers.

The second reason was to deal with the possibility that somebody
might someday devise an interprocedural ThrowAnalysis.  Suppose a
whole program analysis says that the following handler is
unreachable

  try {
    System.out.println("hello");
  } catch (ArithmeticException e) {
    // ... whatever ...
  }

because java.io.PrintStream.println(String) does no integer
divisions. If you remove the handler, though, you've changed the
semantics of the generated class, since it will not behave the
same way should it be executed with a different class library
where java.io.PrintStream.println does perform integer divisions.
Now, if people are going to ask for a whole program analysis,
they really should know that the results are invalidated as soon
as they link against a different set of classes than they
analyzed, so again, my argument that people should have the
option to retain such handlers is not very strong.

I originally added the remove-unreachable-traps option just after
I discovered that code generated from an ExceptionalUnitGraph
could be unverifiable (as a consequence of the bytecode verifier
assuming that all instructions in the scope of a handler have the
potential to throw an exception caught by the handler). My notes
from this period say that I had observed
UnreachableCodeEliminator removing an unreachable handler, after
which PatchingChain redirected the Trap's handlerUnit to the last
unit preceding the handler, resulting in unverifiable code
because the resulting "handler" didn't pop any exception off the
stack.  This story doesn't make any sense to me now, though,
since the Trap corresponding to the eliminated handler should
have been eliminated as well; probably my notes record a mistake I
made because I still didn't understand bytecode verification well
enough to recognize the real circumstances that led to
unverifiable code.

    olhotak> If unreachable traps are left in, it is not possible to produce
    olhotak> bytecode from baf: these traps have no entry points,
    olhotak> so the baf code assumes their initial stack height
    olhotak> is zero, yet the first thing they do is pop the
    olhotak> exception. Does anyone remember why they are not
    olhotak> removed by default?

Is this a "Negative stack height has been attained" exception
being thrown by soot.baf.JasminClass? I dealt with this once
before, with revision 1135, (originally made in the
soot-2-exceptional branch). I changed the mechanism that set the
initial stack heights so that unreachable handlers would have
have an initial height of 1 instead of 0, but it looks like I put
my handler test inside an existing loop which ignored Traps which
trap zero units. Oops. Under the assumption that your test case
involves a handler that is unreachable because the try block is
empty, and not because the ExceptionalUnitGraph has deemed the
handler to be unreachable, then it is my bungled fix to
JasminClass, and not remove-unreachable-traps, that is the
probable cause of your problem.  I'll take another look at it
tomorrow evening.