Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverage: Better branch handling (require/assert) #1967

Closed
Tracked by #1961
onbjerg opened this issue Jun 14, 2022 · 0 comments · Fixed by #2133
Closed
Tracked by #1961

Coverage: Better branch handling (require/assert) #1967

onbjerg opened this issue Jun 14, 2022 · 0 comments · Fixed by #2133
Assignees
Labels
C-forge Command: forge Cmd-forge-coverage Command: forge coverage D-hard Difficulty: hard P-high Priority: high T-feature Type: feature

Comments

@onbjerg
Copy link
Member

onbjerg commented Jun 14, 2022

We don't currently handle require/assert as branches, and if statements with no else branch are considered to be fully covered iff the true path is executed - branches should only be considered to be fully covered if both the true and the false path has been taken.

We also need to figure out how to handle YulSwitch and TryCatchStatement

More context

What makes them more complicated? Doesn't each require/assert(bool) translate to an if (bool) statement and is like a branch?

Ideally for branches we want to have three states: not executed, one path executed (either true or false) or both branches executed (i.e. covered).

Currently for if statements with no else block there are two states: not executed or true branch executed which is not ideal. I think we probably want to signal to the developer if they only covered 1 possible path in their test.

For require and assert though, all of this is marked by specific opcodes, and I need to figure out which. The general layout seems to be:

<some assertion>
PUSH <pc at which we continue execution if assertion holds>
JUMPI
<setup revert and revert>

So my assumption is that the false branch would be right after JUMPI. For the true branch, I'm not sure - maybe I need to record what value is on the stack every time we encounter a JUMPI so I can look it up later and check if we both got a 0 and a non-0 value? :/

Edit: I checked it out on Godbolt btw, here is my contract

// SPDX-License-Identifier: UNLICENSED
pragma solidity >=0.4.0;

contract Test {
    function test(uint256 a) public pure returns (uint256) {
        require(a > 1, "abc");
    }
}

And here is the bytecode for require:

  PUSH 1
  DUP3 
  GT 
  PUSH [tag] test_uint256_6
  JUMPI 
  PUSH 40 # if we end up here, we will end up reverting
# load revert string
  MLOAD 
  PUSH 8C379A000000000000000000000000000000000000000000000000000000000
  DUP2 
  MSTORE 
  PUSH 4
  ADD 
# set return addr
  PUSH [tag] test_uint256_5
  SWAP1
# some abi encoding whatever
  PUSH [tag] abi_encode_tuple_t_stringliteral_4e03657aea45a94fc7d47ba826c8d667c0d1e6e33a64a036ec44f58fa12d6c45__to_t_string_memory_ptr__fromStack_reversed_0
  JUMP [in]
tag test_uint256_5
# after abi encoding we end up here
  JUMPDEST 
  PUSH 40
  MLOAD 
  DUP1 
  SWAP2 
  SUB 
  SWAP1 
  REVERT 
tag test_uint256_6
  JUMPDEST # if we end up here we didn't revert

Originally posted by @onbjerg in #1576 (comment)

@onbjerg onbjerg mentioned this issue Jun 14, 2022
8 tasks
@onbjerg onbjerg added T-feature Type: feature C-forge Command: forge Cmd-forge-coverage Command: forge coverage labels Jun 14, 2022
@onbjerg onbjerg self-assigned this Jun 14, 2022
@onbjerg onbjerg added P-high Priority: high D-hard Difficulty: hard labels Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-coverage Command: forge coverage D-hard Difficulty: hard P-high Priority: high T-feature Type: feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant