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

ICE when copying storage structs to memory arrays via IR #12558

Closed
cameel opened this issue Jan 19, 2022 · 8 comments · Fixed by #12579
Closed

ICE when copying storage structs to memory arrays via IR #12558

cameel opened this issue Jan 19, 2022 · 8 comments · Fixed by #12579
Assignees
Labels
bug 🐛 should compile without error Error is reported even though it shouldn't. Source is fine.

Comments

@cameel
Copy link
Member

cameel commented Jan 19, 2022

Description

Found while working on the external test for Pool Together v4. The contract does not compile via IR at all due to an Internal Compiler Error. With legacy codegen the compilation succeeds.

Steps to Reproduce

Here's a distilled repro for the ICE:

struct S {
    uint x;
}

contract C {
    S sStorage;

    function f() external {
        S[] memory sMemory;

        sMemory[0] = sStorage;
    }
}
Internal compiler error:
/solidity/libsolidity/codegen/ir/IRGeneratorForStatements.cpp(2962): Throw in function solidity::frontend::IRGeneratorForStatements::writeToLValue(const solidity::frontend::IRLValue&, const solidity::frontend::IRVariable&)::<lambda(const solidity::frontend::IRLValue::Memory&)>
Dynamic exception type: boost::wrapexcept<solidity::langutil::InternalCompilerError>
std::exception::what: Solidity assertion failed
[solidity::util::tag_comment*] = Solidity assertion failed

It only happens when the target struct is a part of an array (does not matter whether dynamic or static). Does not happen when the target is just a single struct.

EDIT: When this is fixed, remember to re-enable the ir-optimize-evm+yul preset in the pool-together external test (which is now in develop):

#ir-optimize-evm+yul # FIXME: ICE due to https://github.com/ethereum/solidity/issues/12558

Environment

  • Compiler version: 0.8.11
@cameel cameel added bug 🐛 should compile without error Error is reported even though it shouldn't. Source is fine. labels Jan 19, 2022
@cameel cameel changed the title ICE when copying memory structs to storage arrays via IR ICE when copying storage structs to memory arrays via IR Jan 19, 2022
@ekpyron
Copy link
Member

ekpyron commented Jan 19, 2022

This will happen for any reference type, e.g. the following will also trigger it:

contract C {
    uint[] sStorage;

    function f() external {
        uint[][] memory sMemory;
        sMemory[0] = sStorage;
    }
}

@bshastry
Copy link
Contributor

Co incidentally what I have been trying to get the fuzzer to generate :-)

@ekpyron
Copy link
Member

ekpyron commented Jan 19, 2022

Another one - apparently also doesn't have to be storage :-D...

contract C {
    function f(uint[] calldata a) external {
        uint[][] memory m;
        m[0] = a;
    }
}

For some reason we only took care of memory-to-memory there :-).

@ekpyron
Copy link
Member

ekpyron commented Jan 19, 2022

Actually, I think this is even the same issue as #12220

@cameel
Copy link
Member Author

cameel commented Jan 19, 2022

Just a heads up - I just merged #12560 so when this if fixed, please remember to re-enable the preset that triggers this ICE:

#ir-optimize-evm+yul # FIXME: ICE due to https://github.com/ethereum/solidity/issues/12558

Hopefully it will pass. If not, disable it again and update the comment to state the new reason for disabling it.

@bshastry
Copy link
Contributor

bshastry commented Jan 20, 2022

Co incidentally what I have been trying to get the fuzzer to generate :-)

Short update: Fuzzer reports this case (via different canonical type but that does not matter) now which is somewhat reassuring :-)

contract C {
  function() external[1][] s0;
  constructor(function() external[1][] memory i0)
  {
    i0[0] = s0[1];
  }
}

Edit: Struck through comment regarding function types being uninteresting

@ekpyron
Copy link
Member

ekpyron commented Jan 20, 2022

External function pointers as base type are actually a nice special case to keep in mind (since it's a multi-slot type), so it's a good case to test specifically when fixing this :-).

@chriseth
Copy link
Contributor

chriseth commented Feb 7, 2022

Just a heads up - I just merged #12560 so when this if fixed, please remember to re-enable the preset that triggers this ICE:

re-enabled it here: #12640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 should compile without error Error is reported even though it shouldn't. Source is fine.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants