Re: Soot bugs when first Unit is a target

[Archie Cobbs <archie@dellroad.org>]

Ondrej LHOTAK wrote:
> >     Unit first = (Unit)units.getFirst();
> >     Unit newFirst = Jimple.v().newNopStmt();
> >     units.insertBefore(newFirst, first);
> >     newFirst.redirectJumpsToThisTo(first);
> 
> A less convoluted way to do this would be to use
> units.getNonPatchingChain().insertBefore()

Thanks.

> > For one thing, it's not clear to me what the definition of "head" is
> > in the context of a directed graph. Is a head equivalent to a node with
> > in-degree zero? (I don't think so, but..) If so, then BlockGraph.java is
> > broken, because the first statement is always marked as a head.
> 
> This has come up before. I don't think the definition is clear to
> anybody. It's not defined in DirectedGraph what a "head"/"tail" is.
> In a DirectedGraph, the only definition that makes sense is nodes with
> in/out-degree zero, respectively. Therefore, I believe UnitGraph and
> BlockGraph should not redefine head/tail to mean entry/exit point, but
> instead should define new methods for entry/exit points. This wasn't
> done because nobody commented on it and it would break lots of existing
> code, but IMHO it's more of a fix than a break, so I suggest it be done
> anyway. I'd like to hear other opinions, however. BTW UnitGraph and
> BlockGraph should get a common super-interface ControlFlowGraph with
> these methods (which would be a sub-interface of DirectedGraph).

This sounds better to me. A directed graph (by traditional definition)
has no such thing as a "head" or a "tail". So instead a subclass should
add those concepts, where they should be clearly defined & documented.
I also agree "entry" and "exit" are better terms than "head" and "tail".
Changing the names will also help in quickly finding other code that
needs to be updated :-)

> > FYI, below is my patch to fix related bugs found in the process of
> > investigating this one. Note there is a new fix not included in the
> > previous patch I posted. Maintainers, please review (and commit if
> > acceptable).
> 
> I am grateful for the patches, but they both fail on the current SVN
> version of Soot. I think this is because of John's cleanup of the
> control flow graph building code (revision 1385), which may have fixed
> many of the bugs these patches fix. (I think the heads/tails/entry/exit
> points discussion came up in the context of John's cleanup.)

It looks like BlockGraph.java has been completely rewritten, and
hopefully these bugs have been fixed in the process. I took a look at
1385 and didn't see any obvious mis-assumptions about unit.getPredOf()
always being non-null.

It would be interesting to try the SootBug test on the latest code
and see what happens (see previous email attachment).. my guess is that
BlockGraph.java has been fixed...if so, then you'll get the exception
later in ArrayBoundsCheckerAnalysis instead of BlockGraph.

Thanks,
-Archie

__________________________________________________________________________
Archie Cobbs      *        CTO, Awarix        *      http://www.awarix.com