The optimizer incorrectly removes assembly block that initializes function pointer

[0xkarmacoma]

Description

When using an assembly block to initialize a function pointer with a value provided externally, starting from 0.8.13 the optimizer removes the sstore that initializes it.

Note: this is related to #15707, but the PoC was wrong so I think this deserves a separate issue/discussion. This new PoC shows that the initializer does not always revert.

Steps to reproduce

There was an error in the PoC, the intent was not for the uninitialized call to always revert, but for the constructor argument x to dynamically initialize foo. I know it's weird, but I got this to work on 0.8.12:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.12;

contract A {
    // events
    event Success();

    // storage
    function() internal foo;

    constructor(uint256 x) {
        assembly {
            sstore(foo.slot, x)
        }

        foo();
    }

    // public so that it's not eliminated by the optimizer
    function bar() public {
        emit Success();
    }
}

Manually inspecting the deployment bytecode shows that bar() is at 0x14b, so if we call it like this in a test:

import {A} from "src/A.sol";

contract ATest {
    function test_dyn_fun_ptr() public {
        new A(0x14b << 0x20);
    }
}

then it actually does set and call bar() as expected on 0.8.12:

Ran 1 test for test/A.t.sol:ATest
[PASS] test_dyn_fun_ptr() (gas: 97719)
Traces:
  [97719] ATest::test_dyn_fun_ptr()
    ├─ [65075] → new A@0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f
    │   ├─ emit Success()
    │   └─ ← [Return] 208 bytes of code
    └─ ← [Stop] 

but the sstore is eliminated from 0.8.13 and up as reported previously

Environment

• Compiler version: 0.8.28
• Target EVM version (as per compiler settings): Cancun
• Framework/IDE (e.g. Truffle or Remix): foundry
• EVM execution environment / backend / blockchain client: n/a
• Operating system: Linux/macOS

[ekpyron]

I haven't verified this completely, but I'm assuming you're mixing via-IR and legacy compilation here to produce the different results? (Without having checked yet) with legacy compilation I'd assume that this "works" (i.e. there is an offset to provide to the constructor to call bar) even beyond 0.8.12.
Part of what's going on here is the change in the representation of internal function pointers between legacy and via-IR compilation (see https://docs.soliditylang.org/en/develop/ir-breaking-changes.html#internal-function-pointers), plus more aggressively reducing the retained dispatchable internal functions in via-IR codegen.

Restricting the case to only via-IR, there is still an arguably surprising difference between this slightly adapted piece of code:

contract A {
    // events
    event Success();

    // storage
    function() internal foo;

    constructor(uint256 x) {
        function() internal baz = bar;
        assembly {
            sstore(foo.slot, x)
        }

        foo();
    }

    // public so that it's not eliminated by the optimizer
    function bar() public {
        emit Success();
    }
}

For which there is a constructor value that doesn't revert and call bar (namely 1, the internal function ID assigned to bar) - while there is no such value if the seemingly redundant assignment function() internal baz = bar; is removed.

Technically, this is due to the compiler only retaining code for internal functions that are referenced (as in the function() internal baz = bar; assignment), which is in general quite reasonable.
I'm not entirely sure if it's a reasonable expectation that there are strong guarantees for behaviour when manually writing fully external values to internal function pointers like this, though. Although, admittedly, the current behaviour here can indeed be considered surprising.

[cameel]

I haven't verified this completely, but I'm assuming you're mixing via-IR and legacy compilation here to produce the different results? (Without having checked yet) with legacy compilation I'd assume that this "works" (i.e. there is an offset to provide to the constructor to call bar) even beyond 0.8.12.

I checked that and when compiling via legacy (solc --optimize --asm) the sstore is still there. So the unused store removal itself is not broken, non-referenced function becomes unreachable on IR and that's the reason it gets removed there.

I'm not entirely sure if it's a reasonable expectation that there are strong guarantees for behaviour when manually writing fully external values to internal function pointers like this, though. Although, admittedly, the current behaviour here can indeed be considered surprising.

I'd go as far as to say that persisting the values of internal function pointers in any way (e.g. in storage or as a part of contract's interface) is a questionable design choice, at least in updateable contracts. They are only stable as long as contract's code remains unchanged.

We should at least document it. But perhaps we should also take some measures to prevent this usage in the first place (e.g. by not allowing them to be stored in storage; fortunately they are already forbidden in contract interface).

[cameel]

We discussed this on today's call and decided that we won't consider this a bug. Internal function pointer values are not guaranteed to be stable enough for manipulation via inline assembly to be proper usage. I have now explicitly documented that we don't guarantee something like this to work and also their stability in general: #15721. I also opened a forum thread to gather feedback on further restricting them: [Deprecation feedback] Disallowing internal function pointers in storage.

In any case @karmacoma-eth thanks for the report! While it turned out not to be a bug, it was still pretty useful since it brought our attention to this.

[0xkarmacoma]

thanks for the detailed answer, I appreciate the clarification and the updated docs! definitely agree that this is a questionable practice, but it's good to get some clarity around the expected behavior