[WIP] mark the end of block also with STOP

[hiqua]

I think this change would make sense given the surrounding code and comments, but not completely sure. This fixes the bug with the empty contract:

pragma solidity ^0.4.24;

contract C{
}

Blocked by #57.

[ghost]

DeepCode analyzed this pull request.
There are no new issues.

[ritzdorf]

1. Definitely makes sense. Good catch.
2. Don't you think we should also add Return, Revert and Selfdestruct here? Probably we should define this in a more central place like doesOpcodeEndBlock() and doesOpcodeEndExecution() or so, so that these checks are consistent everywhere.
3. I guess we should also add it here:

   securify/src/main/java/ch/securify/decompiler/Decompiler.java

   Line 183 in f5e7e4a

   || rawInstruction.opcode == OpCodes.JUMPI

   This sparks a more general question, whether we want to keep both of them. (Background: Decompiler is more precise if it works, but function detection rarely does. I don't know why it doesn't work though.) I realized that a couple of recent PRs were only against the fallback variant. (mea culpa)
   @ptsankov was in favour of keeping the Decompiler in, maybe he can elaborate on its advantages.

[hiqua]

I agree with everything you mention, and I have the same questions regarding Decompiler vs DecompilerFallback.

I think I'll first focus on having more tests before we do these changes, so that we can keep track of whether we introduce bugs or not.

[hiqua]

For STOP it is clear that it fixes some things using the examples I have added on another branch (basically Securify does not fail on SafeMath anymore), I'm looking for examples for the other opcodes now so that we can compare.

[ritzdorf]

Isn't this an example for REVERT?

pragma solidity ^0.4.24;
contract C{
  function() public payable{
    revert();
  }
}

[hiqua]

Yes, and it works also for the other opcodes, so perfect.

[hiqua]

I've added the other opcodes and tests, if the changes are good for you I'll reorganize the commits and merge.

[ritzdorf]

Why does JUMPI not end a block?

[hiqua]

In the code there is a check on controlFlowGraph.get(rawInstruction.offset).contains(ControlFlowDetector.DEST_ERROR) when a JUMPI is encountered, meaning that Securify currently does not assume that this is the end of the block just because of this opcode. Ignoring JUMPI in this function was a way to refactor the code without changing the semantics of the basic blocks Securify currently builds.

[ritzdorf]

Can you briefly explain the changes that are going on in the 238 changed files? How does the end-to-end testing work? From where is it triggered?

[hiqua]

Can you briefly explain the changes that are going on in the 238 changed files? How does the end-to-end testing work? From where is it triggered?

Sorry that should be clearer now, basically I want to wait until the end_to_end_testing branch is merged before we merge this. It just compares the current output with saved output so that we manually acknowledge the changes. See #57 for more details.

[hiqua]

See #68.