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

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

Closed
Fraccaman opened this issue May 2, 2022 · 23 comments · Fixed by #843
Closed

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

Fraccaman opened this issue May 2, 2022 · 23 comments · Fixed by #843

Comments

@Fraccaman
Copy link

Fraccaman commented May 2, 2022

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
Copy link
Member

cgewecke commented May 2, 2022

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
Copy link
Author

Fraccaman commented May 2, 2022

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
Copy link
Author

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

@cgewecke
Copy link
Member

cgewecke commented May 3, 2022

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.

@cgewecke cgewecke changed the title Zero coverage on contracts, 100% on interfaces Setting viaIR to true in compiler settings results in zero coverage May 3, 2022
@Fraccaman
Copy link
Author

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

@aleph-v
Copy link

aleph-v commented Jun 27, 2022

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
Copy link

Xutp-F commented Oct 10, 2022

hello,do you need to write tests manually?

@jamesmorgan
Copy link

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

@jmendiola222
Copy link

jmendiola222 commented Jan 3, 2023

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: ethereum/solidity#11638 (comment), same result:

optimizer: {
  enabled: true,
  runs: 200,
  details: {
    yul: true,
  },
 },
 viaIR: false,

@ZumZoom
Copy link
Contributor

ZumZoom commented Feb 15, 2023

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
Copy link

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
Copy link

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

@jmendiola222
Copy link

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
Copy link
Contributor

ZumZoom commented Jul 12, 2023

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

@lienhage
Copy link

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
Copy link

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 🥳 ! 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
Copy link

jmendiola222 commented Jul 28, 2023

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
Copy link

A potential workaround to identify the problematic contracts here: #760 (comment)

@remedcu
Copy link
Contributor

remedcu commented Dec 5, 2023

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.

remedcu added a commit to BlockchainAsset/solidity-coverage that referenced this issue Dec 5, 2023
@rsimonton
Copy link

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

@cgewecke cgewecke mentioned this issue Jan 11, 2024
14 tasks
@cgewecke cgewecke reopened this Feb 2, 2024
@cgewecke cgewecke added the viaIR label Feb 4, 2024
@cgewecke cgewecke changed the title Setting viaIR to true in compiler settings results in zero coverage Setting viaIR to true in solc results in zero coverage Feb 4, 2024
@cgewecke
Copy link
Member

cgewecke commented Feb 9, 2024

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
Copy link
Member

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

@cgewecke cgewecke unpinned this issue Feb 10, 2024
@jmendiola222
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.