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

Solidity Assembly allowing access to immutables #13912

Closed
k06a opened this issue Feb 1, 2023 · 10 comments
Closed

Solidity Assembly allowing access to immutables #13912

k06a opened this issue Feb 1, 2023 · 10 comments

Comments

@k06a
Copy link

k06a commented Feb 1, 2023

Abstract

I understand the concern of immutable access related to missing CODELOAD opcode and having side effects of CODECOPY, But I think we still can have some sort of the access.

Motivation

Currently access to immutable is possible via copying immutable values to local variables, but it increases stack size which could cause "Stack too deep" issues.

Specification

I consider immutable variable in assembly block could reference to "code offset" and developers could manually handle the code copying into memory like that:

contract Foo {
    address public immutable token;

    constructor(address token_) {
        token = token_;
    }

    function f() {
        assembly {
            codecopy(0, token.offset, 0x20)
            let t := mload(0) // <-- access to immutable value
        }
    }
}
@hrkrshnn
Copy link
Member

hrkrshnn commented Feb 3, 2023

Currently access to immutable is possible via copying immutable values to local variables, but it increases stack size which could cause "Stack too deep" issues.

Did you try using via-ir? I think these issues are more or less solved there. This cannot be the sole reason for implementing this feature.

@k06a
Copy link
Author

k06a commented Feb 4, 2023

@hrkrshnn sure, unfortunately in our cases via-ir option produces more issues related to “Stack too deep”, especially with “coverage” flow.

@0xmikko
Copy link

0xmikko commented Feb 6, 2023

Looks very useful from my side, it removes barrier to rewrite some parts of code using jul

@ekpyron
Copy link
Member

ekpyron commented Feb 6, 2023

Ultimately, whatever you do in assembly, you're bound to consume at least one stack slot as well for it, if not more - copying the immutable to a local variable will only end up as a push-constant, so I can't think of any situation in which that'd be better stack-wise and runtime-gas-wise...

Are the issues you're having with via-IR due to running it without optimizer? If so, did you try to enable the optimizer with a minimal yul optimizer sequence instead (we may make that the default in the future to avoid confusion here)?

@pcaversaccio
Copy link

A quick Q: could someone quickly explain which of the optimizer passes should eliminate the stack-too-deep errors exactly? Am still having a hard time fully grasping it.

I might be wrong here, but one use case where I think it could be beneficial is where you use the scratch space for something like keccak256(bytes.concat(a, b)) with a and b being 32 bytes each and either a or b is an immutable variable that can be accessed easily with this feature.

@cameel
Copy link
Member

cameel commented Feb 6, 2023

A quick Q: could someone quickly explain which of the optimizer passes should eliminate the stack-too-deep errors exactly? Am still having a hard time fully grasping it.

It's the stack to memory mover that moves variables to memory. It does not have a specific step assigned. You should get this effect even just by enabling the optimizer but with the sequence being completely empty.

@pcaversaccio
Copy link

I see - is there any combination of sequences that could resolve a stack-too-deep error after running the normal optimizer steps? Or in other words, even with the via-ir pipeline and optimizer enabled, using the default steps, I sometimes face stack-too-deep errors. What additional optimizer steps, if possible, I could add to resolve it? My usual way of solving this is using structs currently.

@cameel
Copy link
Member

cameel commented Feb 6, 2023

In general doing less inlining might make them go away but that would usually be an overkill and may also make your code less efficient overall just to work around a problem at a single spot.

You may be able to get rid of the problem by moving code around instead - these issues are usually pretty ephemeral and just changing the order or calls or sizes of variables or struct fields can make them randomly go away, which may be good enough to solve some localized instances of it when it's just a small recent change causing it.

Unfortunately there's no magical step to fix this problem. Or rather, the magical step is the stack to memory mover and it's always applied, independently of what other steps you choose. If you're still getting "stack to deep" with via-ir and optimizer enabled and you have nothing in your code that would prevent the mover from being applied (recursive functions or non-memory-safe assembly blocks), then this is something that we need to fix in the compiler.

In that case the best you can do is to help us track down the problem. Trimming you code to a small repro like #13906 (comment) and submitting a bug helps a lot.

@pcaversaccio
Copy link

thx for elaborating.

In that case the best you can do is to help us track down the problem. Trimming you code to a small repro like #13906 (comment) and submitting a bug helps a lot.

Will do.

@ekpyron
Copy link
Member

ekpyron commented Feb 7, 2023

Back to the original issue here: the way to do that would be to expose loadimmutable to inline assembly - but since that mechanism is actually rather weird and bound to still change, I'd rather not expose it further than it needs to be right now. Given the discussion, I actually don't think there needs to be assembly access to immutables, before #13723 / #13323 - so I'm closing this as a consideration for/after the implementation of those instead. Feel free to reopen if there is compelling need for this earlier than that.

@ekpyron ekpyron closed this as completed Feb 7, 2023
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

No branches or pull requests

6 participants