Skip to content

make progress on stack depth issues when running coverage #75

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

Merged
merged 13 commits into from
Apr 16, 2025

Conversation

0age
Copy link
Collaborator

@0age 0age commented Apr 14, 2025

The goal here is to run forge coverage which compiles the code without the IR optimization step (note that forge coverage --ir-minimum does not cover the inline assembly so isn't going to work).

There are a few places where the code gets uglier for sure, but the actual functionality and efficiency should be intact with the exception of a few gas here and there for an extra calldataload / pop / etc. I think the value of having coverage in place is worth it (with the caveat that once we are comfortable with the coverage in place we can always change things back).

however I'm now hitting a blocker on this dreadful error:

Error: Compiler error (/solidity/libyul/backends/evm/AsmCodeGen.cpp:68):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. When compiling inline assembly: Variable headStart is 1 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.

best I can gather is that this means there's some function that has too many arguments which means the function can't even be entered or called at all; if anyone has thoughts on how we might track it down and make progress here it'd be much appreciated!

@0age
Copy link
Collaborator Author

0age commented Apr 14, 2025

ok so this error is originating from both of the permit2 depositAndRegister functions

even with no named variables and no function body, the error still gets tripped

we have a number of options for these:

  • (obviously) change the function interface a bit, e.g. replace some distinct args with a struct or pack a few arguments
  • work out some minimal set of steps we can take to address this at the compiler level while still keeping coverage (everything I've tried so far has come up short here)
  • mine a function name that takes fewer arguments and calldatacopy the others where relevant; relatedly, would need to drop these from the interface (this feels really hacky to me)

will plan to modify the interface to get this working for now, which seems fine since we already changed it for the lockTag adjustment; open to other suggestions though

@0age 0age marked this pull request as ready for review April 16, 2025 17:23
@0age 0age requested review from zeroknots and reednaa and removed request for zeroknots April 16, 2025 17:26
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@0age 0age merged commit 12be945 into v1 Apr 16, 2025
2 checks passed
@0age 0age deleted the coverage-stack-juggling branch April 16, 2025 19:41
Copy link
Collaborator

@reednaa reednaa left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -264,6 +257,19 @@ contract TheCompact is ITheCompact, ERC6909, TheCompactLogic {
bytes32 witness,
bytes calldata sponsorSignature
) external returns (bytes32 claimHash) {
_enforceConsistentAllocators(idsAndAmounts);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: We may be looping over idsAndAmounts twice in this function. Once in _enforceConsistentAllocators and once in .hasValidSponsor if the emissary is checked. Because of stack issues, I don't believe we want to fix.

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

Successfully merging this pull request may close these issues.

4 participants