Skip to content
New issue

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

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

Already on GitHub? # to your account

fix(coverage): add new option "--ir-minimum" to resolve the "stack too deep" error #5349

Merged

Conversation

spockP
Copy link
Contributor

@spockP spockP commented Jul 10, 2023

Motivation

Partially fix #3357.

More and more people (including myself) are using the magical via-ir to solve the "stack too deep" issue. However, this would cause problems in test coverage as all the optimizations are disabled in test coverage.

The solidity team suggests working around this issue by enabling "via-ir" with a "minimum amount of optimization."

And this workaround is also recommended in the FAQ of solcoverage.

Solution

In this pr, a new "--ir-minimum" option is added to the coverage subcommand, which enables "via-ir" with a minimum amount of optimization.

test case

The contract Deep in deep.sol will not compile without any optimizations due to the "stack too deep" error:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

contract Deep {
    mapping(uint256 => uint256) private numbers;

    function sum() public view returns (uint256) {
        uint256 x0 = numbers[0];
        uint256 x1 = numbers[1];
        uint256 x2 = numbers[2];
        uint256 x3 = numbers[3];
        uint256 x4 = numbers[4];
        uint256 x5 = numbers[5];
        uint256 x6 = numbers[6];
        uint256 x7 = numbers[7];
        uint256 x8 = numbers[8];
        uint256 x9 = numbers[9];
        uint256 x10 = numbers[10];
        uint256 x11 = numbers[11];
        uint256 x12 = numbers[12];
        uint256 x13 = numbers[13];
        uint256 x14 = numbers[14];
        uint256 x15 = numbers[15];
        uint256 x16 = numbers[16];
        uint256 x17 = numbers[17];

        // return sum of all numbers
        return x0 + x1 + x2 + x3 + x4 + x5 + x6 + x7 + x8 + x9 + x10 + x11 + x12 + x13 + x14 + x15 + x16 + x17;
    }
}

And the test script is as follows:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/deep.sol";

contract TestA is Test {
    Deep sut;

    function setUp() external {
        sut = new Deep();
    }

    function test_GetA() external {
        uint256 result = sut.sum();
        assertEq(result, 0);
    }
}

When you run test coverage without "--ir-minimum", you will see the "stack too deep" error:

spock@local:~/case$ forge coverage 
[⠆] Compiling...
[⠊] Compiling 19 files with 0.8.20
[⠢] Solc 0.8.20 finished in 3.67s
Error: 
Compiler run failed:
Error: Compiler error (/solidity/libsolidity/codegen/LValue.cpp:56):Stack too deep. Try compiling with `--via-ir` (cli) or the equivalent `viaIR: true` (standard JSON) while enabling the optimizer. Otherwise, try removing local variables.
  --> src/deep.sol:28:61:
   |
28 |         return x0 + x1 + x2 + x3 + x4 + x5 + x6 + x7 + x8 + x9 + x10 + x11 + x12 + x13 + x14 + x15 + x16 + x17;
   |        

When you run test coverage with "--ir-minimum", the "stack too deep" error is gone:

spock@local:~/case$ forge coverage --ir-minimum  
[⠑] Compiling...
[⠢] Compiling 19 files with 0.8.20
[⠰] Solc 0.8.20 finished in 3.53s
Compiler run successful!
Analysing contracts...
Running tests...
| File         | % Lines         | % Statements    | % Branches    | % Funcs       |
|--------------|-----------------|-----------------|---------------|---------------|
| src/deep.sol | 100.00% (19/19) | 100.00% (20/20) | 100.00% (0/0) | 100.00% (1/1) |
| Total        | 100.00% (19/19) | 100.00% (20/20) | 100.00% (0/0) | 100.00% (1/1) |

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this—but we might want to add a CLI warning that adding this flag overrides several configuration options and should probably only be used as a workaround for stack too deep, and link to the relevant issue.

wdyt @mattsse ?

@mattsse
Copy link
Member

mattsse commented Jul 11, 2023

should be unblocked now

Comment on lines +120 to +128
p_println!(!self.opts.silent => "{}",
Paint::yellow(
concat!(
"Warning! \"--ir-minimum\" flag enables viaIR with minimum optimization, which can result in inaccurate source mappings.\n",
"Only use this flag as a workaround if you are experiencing \"stack too deep\" errors.\n",
"Note that \"viaIR\" is only available in Solidity 0.8.13 and above.\n",
"See more:\n",
"https://github.com/foundry-rs/foundry/issues/3357\n"
)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Evalir CLI warning added

Comment on lines +108 to +110
// TODO: How to detect solc version if the user does not specify a solc version in
// config case1: specify local installed solc ?
// case2: mutliple solc versions used and auto_detect_solc == true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vir-ir and ir-minimum will only work in solidity 0.8.13 and above.
I don't find a better way to reject the unsatisfied compilers.

@spockP spockP force-pushed the spock/coverage-via-ir-minimum-optimization branch from 74de551 to ce860a1 Compare July 12, 2023 13:43
@mattsse mattsse requested a review from Evalir July 12, 2023 14:04
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good now! thanks!

if let Some(SolcReq::Version(version)) = &config.solc {
if *version < Version::new(0, 8, 13) {
return Err(eyre::eyre!(
"viaIR with minimum optimization is only available in Solidity 0.8.13 and above."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great!

@spockP spockP requested a review from mattsse July 13, 2023 01:52
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tysm for this

Comment on lines +108 to +110
// TODO: How to detect solc version if the user does not specify a solc version in
// config case1: specify local installed solc ?
// case2: mutliple solc versions used and auto_detect_solc == true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can ignore this this,

all options are sanitized in ethers-solc depending on version, meaning unsupported options are removed before calling solc

@mattsse mattsse merged commit 30052ee into foundry-rs:master Jul 13, 2023
@davidlucid
Copy link

davidlucid commented Oct 28, 2023

Hey guys!

I have I seem to have found a bug with forge coverage --ir-minimum. When running forge coverage alone, I get a "stack too deep" error; when running forge coverage --ir-minimum, it works, but it tells me I don't have coverage on an area of my tests that I clearly have coverage for (specifically, 3 lines of assembly code for loading data into memory). I know I have coverage for these 3 lines because if you remove some of the contract files and one of the test files, the normal forge coverage command works and tells me I have 100% coverage (but again tells me I don't have 100% coverage if I run forge coverage --ir-minimum).

Any ideas on what to do here to get coverage to show 100%?

I have posted a bug report issue for this here FYI: #6156

Thanks! @mattsse @Evalir @spockP

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(forge coverage): stack too deep when using --ir-minimum and coverage report is not accurate
4 participants