sc-forks / solidity-coverage

Code coverage for Solidity smart-contracts
MIT License
970 stars 261 forks source link

Setting `viaIR` to true in solc results in zero coverage #715

Closed Fraccaman closed 7 months ago

Fraccaman commented 2 years ago

Following the docs here, and running npx hardhat coverage gives this result:

Version
=======
> solidity-coverage: v0.7.21

Instrumenting for coverage...
=============================

> contract/Bridge.sol
> contract/Governance.sol
> contract/Hub.sol
> interface/IBridge.sol
> interface/ICommon.sol
> interface/IGovernance.sol
> interface/IHub.sol

Compilation:
============

Nothing to compile

Network Info
============
> HardhatEVM: v2.9.3
> network:    hardhat

  Bridge
    ✔ Initialize contract testing (84ms)
    ✔ Update validate set testing (749ms)
    ✔ Authorize testing

  Governance
    ✔ Initialize contract testing (44ms)
    ✔ Update validator set testing (429ms)
    ✔ Upgrade contract testing (67ms)
    ✔ Add contract testing (69ms)

  Hub
    ✔ Initialize contract testing (157ms)

  8 passing (5s)

------------------|----------|----------|----------|----------|----------------|
File              |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
------------------|----------|----------|----------|----------|----------------|
 contract/        |        0 |        0 |        0 |        0 |                |
  Bridge.sol      |        0 |        0 |        0 |        0 |... 335,345,355 |
  Governance.sol  |        0 |        0 |        0 |        0 |... 229,230,233 |
  Hub.sol         |        0 |        0 |        0 |        0 |... 84,88,92,96 |
 interface/       |      100 |      100 |      100 |      100 |                |
  IBridge.sol     |      100 |      100 |      100 |      100 |                |
  ICommon.sol     |      100 |      100 |      100 |      100 |                |
  IGovernance.sol |      100 |      100 |      100 |      100 |                |
  IHub.sol        |      100 |      100 |      100 |      100 |                |
------------------|----------|----------|----------|----------|----------------|
All files         |        0 |        0 |        0 |        0 |                |
------------------|----------|----------|----------|----------|----------------|

> Istanbul reports written to ./coverage/ and ./coverage.json

which is weird. My .solcover.js content is:

module.exports = {
    configureYulOptimizer: true
};

Not sure what I'm missing.

cgewecke commented 2 years ago

Could you try running npx hardhat clean && npx hardhat coverage?

Also if you could share your config or a reproducible example that would be helpful.

Fraccaman commented 2 years ago

The output didn't change :(

npx hardhat clean && npx hardhat coverage                                                                                                                    ✔  12.22.1   18:51:27

Version
=======
> solidity-coverage: v0.7.21

Instrumenting for coverage...
=============================

> contract/Bridge.sol
> contract/Governance.sol
> contract/Hub.sol
> interface/IBridge.sol
> interface/ICommon.sol
> interface/IGovernance.sol
> interface/IHub.sol

Compilation:
============

Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
   --> contracts/contract/Bridge.sol:333:9:
    |
333 |         ValidatorSetArgs calldata validatorSetArgs,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
   --> contracts/contract/Bridge.sol:334:9:
    |
334 |         Signature[] calldata signatures,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Warning: Unused function parameter. Remove or comment out the variable name to silence this warning.
   --> contracts/contract/Bridge.sol:335:9:
    |
335 |         address to
    |         ^^^^^^^^^^

Warning: Function state mutability can be restricted to pure
   --> contracts/contract/Bridge.sol:332:5:
    |
332 |     function withdraw(
    |     ^ (Relevant source part starts here and spans across multiple lines).

Compiled 14 Solidity files successfully

Network Info
============
> HardhatEVM: v2.9.3
> network:    hardhat

  Bridge
    ✔ Initialize contract testing (92ms)
    ✔ Update validate set testing (761ms)
    ✔ Authorize testing

  Governance
    ✔ Initialize contract testing (39ms)
    ✔ Update validator set testing (444ms)
    ✔ Upgrade contract testing (65ms)
    ✔ Add contract testing (69ms)

  Hub
    ✔ Initialize contract testing (155ms)

  8 passing (5s)

------------------|----------|----------|----------|----------|----------------|
File              |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
------------------|----------|----------|----------|----------|----------------|
 contract/        |        0 |        0 |        0 |        0 |                |
  Bridge.sol      |        0 |        0 |        0 |        0 |... 335,345,355 |
  Governance.sol  |        0 |        0 |        0 |        0 |... 229,230,233 |
  Hub.sol         |        0 |        0 |        0 |        0 |... 84,88,92,96 |
 interface/       |      100 |      100 |      100 |      100 |                |
  IBridge.sol     |      100 |      100 |      100 |      100 |                |
  ICommon.sol     |      100 |      100 |      100 |      100 |                |
  IGovernance.sol |      100 |      100 |      100 |      100 |                |
  IHub.sol        |      100 |      100 |      100 |      100 |                |
------------------|----------|----------|----------|----------|----------------|
All files         |        0 |        0 |        0 |        0 |                |
------------------|----------|----------|----------|----------|----------------|

> Istanbul reports written to ./coverage/ and ./coverage.json

Ill setup a reproducible example and report back! Thanks for the quick response :)

Fraccaman commented 2 years ago

@cgewecke here you go! Thanks again for your help!

cgewecke commented 2 years ago

Thanks for the repro. If I comment out viaIR in your compiler settings as below I get the expected report. You might want to set this conditionally using an environment variable that's toggled on when coverage runs.

solidity: {
    version: "0.8.13",
    settings: {
      optimizer: {
        enabled: true,
        runs: 200,
      },
      //viaIR: true,
    },
  },

Report Screen Shot 2022-05-03 at 11 04 13 AM

[EDIT - For clarification, some optimizer settings remove the instrumentation soldiity-coverage injects into the contracts to track the execution trace. I've edited your issue title to reflect what I think the underlying cause is so other people can easily find it.]

TODO: set viaIR to false by default.

Fraccaman commented 2 years ago

ah, I see! thanks for the suggestion, works like a charm now :)

aleph-v commented 2 years ago

Hi @cgewecke, do y'all have plans to support coverage with viaIR == true? The default of turning off viaIR for our code means coverage cannot run because viaIR eliminates stack depth too deep errors. Example error from our code:

Compilation:
============
YulException: Variable var__143_mpos is 3 slot(s) too deep inside the stack.
Xutp-F commented 1 year ago

hello,do you need to write tests manually?

jamesmorgan commented 1 year ago

Any updates on this - would be great to get coverage back in action for viaIR projects

jmendiola222 commented 1 year ago

Hi, any progress on this one? viaIR has become a MUST on "stack too deep" and >24kb sized contracts. Turning off vialR for coverage it's not longer an options if the contract does not compile due to stack too deep...
I don't think the has-workaround label really applies @cgewecke

I'm using this on .solccover.js:

module.exports = {
    ...
    configureYulOptimizer: true,
    solcOptimizerDetails: {
      peephole: false,
      inliner: false,
      jumpdestRemover: false,
      orderLiterals: true,  // <-- TRUE! Stack too deep when false
      deduplicate: false,
      cse: false,
      constantOptimizer: false,
      yul: false
    },
}

But still getting (without ViaIR):

CompilerError: 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.
    --> contracts/MyContract.sol:1027:21:

Error in plugin solidity-coverage: HardhatError: HH600: Compilation failed

And with ViaIR:

YulException: Variable headStart is 2 slot(s) too deep inside the stack. 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.

Using solidity-coverage v 8.13.1

Here you can see a repo to reproduce the error, it compiles fine, but coverage does not run.

Also tried yul: true as suggested here: https://github.com/ethereum/solidity/issues/11638#issuecomment-1301656139, same result:

optimizer: {
  enabled: true,
  runs: 200,
  details: {
    yul: true,
  },
 },
 viaIR: false,
ZumZoom commented 1 year ago

I've found a decent workaround for the issue:

As @cgewecke mentioned some optimiser settings remove the instrumentation soldiity-coverage injects, so we just need to disable those settings. Default yul-optimizer steps can be found in the solidity repo and the rules that remove instrumentation are u and i. Since there are no u and i steps in the cleanup steps we can keep those default.

With this in mind I've set my .solcover.js to look like this:

module.exports = {
    configureYulOptimizer: true,
    solcOptimizerDetails: {
        yul: true,
        yulDetails: {
            optimizerSteps:
                "dhfoDgvlfnTUtnIf" +               // None of these can make stack problems worse
                "[" +
                    "xa[r]EscLM" +                 // Turn into SSA and simplify
                    "cCTUtTOntnfDIl" +             // Perform structural simplification
                    "Lcl" +                        // Simplify again
                    "Vcl [j]" +                    // Reverse SSA

                    // should have good "compilability" property here.

                    "Tpel" +                       // Run functional expression inliner
                    "xa[rl]" +                     // Prune a bit more in SSA
                    "xa[r]cL" +                    // Turn into SSA again and simplify
                    "gvf" +                        // Run full inliner
                    "CTUca[r]LSsTFOtfDnca[r]Ilc" + // SSA plus simplify
                "]" +
                "jml[jl] VcTOcl jml",
        },
    },
}

And now the coverage works for builds with yul optimizer.

jmendiola222 commented 1 year ago

It didn't worked for me @ZumZoom, it took literally hours to compile, and when it ran, it throws:

Error: invalid bytecode (argument="bytecode", value="..."
code=INVALID_ARGUMENT, version=contracts/5.7.0)
    at Logger.makeError (/node_modules/@ethersproject/logger/src.ts/index.ts:269:28)
    at Logger.throwError (/node_modules/@ethersproject/logger/src.ts/index.ts:281:20)
    at Logger.throwArgumentError (node_modules/@ethersproject/logger/src.ts/index.ts:285:21)
    at new ContractFactory (/node_modules/@ethersproject/contracts/src.ts/index.ts:1172:20)
    at _deploy (/node_modules/hardhat-deploy/src/helpers.ts:568:17)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async _deployOne (/node_modules/hardhat-deploy/src/helpers.ts:1004:16)
hung-native commented 1 year ago

I have the same issue and the workarounds above doesn't work for me either.

jmendiola222 commented 1 year ago

Can we remove the has-workaround label please? I might be blocking this issue to be worked on, as in reality the workaround seems to only had worked for @ZumZoom. Thanks.

ZumZoom commented 1 year ago

We've tried this approach on a bunch of more repos, and we discovered two more things:

So now the .solcover.js that we use looks like this:

module.exports = {
    configureYulOptimizer: true,
    solcOptimizerDetails: {
        yul: true,
        yulDetails: {
            optimizerSteps:
                "dhfoDgvlfnTUtnIf" +              // None of these can make stack problems worse
                "[" +
                    "xa[r]EsLM" +                 // Turn into SSA and simplify
                    "CTUtTOntnfDIl" +             // Perform structural simplification
                    "Ll" +                        // Simplify again
                    "Vl [j]" +                    // Reverse SSA

                    // should have good "compilability" property here.

                    "Tpel" +                       // Run functional expression inliner
                    "xa[rl]" +                     // Prune a bit more in SSA
                    "xa[r]L" +                     // Turn into SSA again and simplify
                    "gvf" +                        // Run full inliner
                    "CTUa[r]LSsTFOtfDna[r]Il" +    // SSA plus simplify
                "]" +
                "jml[jl] VTOl jml : fDnTO",
        },
    },
}

Long compile time is sort of viaIR feature, but it seems that removing inlining and pruning makes the process even slower than with default config.

@jmendiola222 Can you please try this config in your project?

Edit: I've tried my workaround on your example, and it works and coverage produces valid results. You can check here: https://github.com/ZumZoom/solidity-coverage-via-ir

lienhage commented 1 year ago
module.exports = {
    configureYulOptimizer: true,
    solcOptimizerDetails: {
        yul: true,
        yulDetails: {
            optimizerSteps:
                "dhfoDgvlfnTUtnIf" +              // None of these can make stack problems worse
                "[" +
                    "xa[r]EsLM" +                 // Turn into SSA and simplify
                    "CTUtTOntnfDIl" +             // Perform structural simplification
                    "Ll" +                        // Simplify again
                    "Vl [j]" +                    // Reverse SSA

                    // should have good "compilability" property here.

                    "Tpel" +                       // Run functional expression inliner
                    "xa[rl]" +                     // Prune a bit more in SSA
                    "xa[r]L" +                     // Turn into SSA again and simplify
                    "gvf" +                        // Run full inliner
                    "CTUa[r]LSsTFOtfDna[r]Il" +    // SSA plus simplify
                "]" +
                "jml[jl] VTOl jml : fDnTO",
        },
    },
}

tried but didn't work for me

jmendiola222 commented 1 year ago

We've tried this approach on a bunch of more repos, and we discovered two more things:

* we had to remove `c` sometimes

* because of the previous finding we also had to add custom cleanup sequence with removed `c`

So now the .solcover.js that we use looks like this:

module.exports = {
    configureYulOptimizer: true,
    solcOptimizerDetails: {
        yul: true,
        yulDetails: {
            optimizerSteps:
                "dhfoDgvlfnTUtnIf" +              // None of these can make stack problems worse
                "[" +
                    "xa[r]EsLM" +                 // Turn into SSA and simplify
                    "CTUtTOntnfDIl" +             // Perform structural simplification
                    "Ll" +                        // Simplify again
                    "Vl [j]" +                    // Reverse SSA

                    // should have good "compilability" property here.

                    "Tpel" +                       // Run functional expression inliner
                    "xa[rl]" +                     // Prune a bit more in SSA
                    "xa[r]L" +                     // Turn into SSA again and simplify
                    "gvf" +                        // Run full inliner
                    "CTUa[r]LSsTFOtfDna[r]Il" +    // SSA plus simplify
                "]" +
                "jml[jl] VTOl jml : fDnTO",
        },
    },
}

Long compile time is sort of viaIR feature, but it seems that removing inlining and pruning makes the process even slower than with default config.

@jmendiola222 Can you please try this config in your project?

Edit: I've tried my workaround on your example, and it works and coverage produces valid results. You can check here: https://github.com/ZumZoom/solidity-coverage-via-ir

Hey, it did work :partying_face: ! It took and hour to compile, and a couple more to run, 4 tests failed (no clear reason why) but overall all the rest worked. Thanks @ZumZoom

jmendiola222 commented 1 year ago

Update, even if the workaround technically works, it's quite inconvenient, for example our CI failed with:

The job running on runner GitHub Actions 2 has exceeded the maximum execution time of 360 minutes.

On dev's 8core/16Gb PCs also takes ~8hs to complete, and almost making it unusable to any other task. As reference, running the tests (also with VIAIR enable) takes ~1hs.

zarifpour commented 11 months ago

A potential workaround to identify the problematic contracts here: https://github.com/sc-forks/solidity-coverage/issues/760#issuecomment-1717775797

remedcu commented 9 months ago

For me, the below code in solcover.js worked:

    ...
    configureYulOptimizer: true,
    solcOptimizerDetails: {
        yul: true,
        yulDetails: {
            optimizerSteps: ""
        },
    },
    ...

Solidity: 0.8.19 Hardhat Compiler Settings: {"viaIR":true,"optimizer":{"enabled":true, "details": {"yul": true}}}

However, some tests use higher gas compared to just running tests. So, might need to change some tests accordingly.

rsimonton commented 9 months ago

Please remove the has-workaround label as the workaround isn't viable.

cgewecke commented 7 months ago

A patch for this problem has been published at the viaIR tag

npm install --save-dev solidity-coverage@viaIR

Should also be live in the next release (0.8.7). If you're using either of those versions, you can remove any special solc config or solc related options (like configureYulOptimizer) from .solcover.js and compile normally.

Gas consumption has changed slightly when using viaIR compatible instrumentation - if you're looking for specific values in your tests you might need to update them.

854 was E2E tested on 1inch's solidity-utils and everything looked ok. Coverage is the same etc.

Apologies for how long it took to fix this, I didn't think there was a way to do it, but in the end there was. Also thank you for sharing your solc configs.

cgewecke commented 7 months ago

This should be fixed in v0.8.7 - viaIR is natively supported and no special config is required.

If you're still running into issues with this please report them to #861

jmendiola222 commented 6 months ago

Fixed on v0.8.7 for us indeed, and the time drooped to ~2x normal test run (from ~8x with the "hack").