-
Notifications
You must be signed in to change notification settings - Fork 6.3k
Remove the wrong codegen assumption that a conversion between non-byte calldata arrays can never happen #16415
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR is missing a changelog entry.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| // SPDX-License-Identifier: GPL-3.0 | ||
| pragma solidity >= 0.0.0; | ||
|
|
||
| contract C { | ||
| function g_int(uint[] calldata a) public pure returns(uint[] memory, uint[] calldata) { | ||
| uint[] memory ops1; // we take two arrays so the compiler won't short-circuit the conditional | ||
| uint[] memory ops2; | ||
|
|
||
| (uint[] memory a1, uint[] calldata b1) = true ? (ops1, a) : (ops2, a); | ||
|
|
||
| return (a1, b1); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| #!/usr/bin/env python3 | ||
|
|
||
| import os | ||
| import sys | ||
| import subprocess | ||
| from pathlib import Path | ||
| from textwrap import dedent | ||
|
|
||
| # pylint: disable=wrong-import-position | ||
| PROJECT_ROOT = Path(__file__).parents[3] | ||
| sys.path.insert(0, str(PROJECT_ROOT / 'scripts')) | ||
|
|
||
| from common.cmdline_helpers import add_preamble | ||
| from common.cmdline_helpers import inside_temporary_dir | ||
| from common.cmdline_helpers import save_bytecode | ||
| from common.cmdline_helpers import solc_bin_report | ||
| from common.git_helpers import git_diff | ||
| from splitSources import split_sources | ||
|
|
||
|
|
||
| @inside_temporary_dir(Path(__file__).parent.name) | ||
| def test_no_convert_fun_generated(): | ||
| source_file_path = Path(__file__).parent / 'inputs.sol' | ||
| add_preamble(Path.cwd()) | ||
|
|
||
| solc_binary = os.environ.get('SOLC') | ||
| if solc_binary is None: | ||
| raise RuntimeError(dedent("""\ | ||
| `solc` compiler not found. | ||
| Please ensure you set the SOLC environment variable | ||
| with the correct path to the compiler's binary. | ||
| """)) | ||
|
|
||
| output = subprocess.check_output( | ||
| [solc_binary] | ||
| + [source_file_path] | ||
| + (["--ir"]) | ||
| + (["--optimize"]), | ||
| encoding="utf8", | ||
| ) | ||
|
|
||
| # This test verifies that the compiler does not generate a function converting between tuples of the same types. | ||
| assert "function convert_" not in output, "Output should not contain 'function convert_*'" | ||
|
|
||
| return 0 | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| sys.exit(test_no_convert_fun_generated()) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| pragma abicoder v2; | ||
|
|
||
| contract C { | ||
| function g_int(uint[] calldata a) public returns(uint, uint, uint, uint) { | ||
| (uint[] calldata b, ) = true ? (a, 0) : (a, 0); | ||
| return (b.length, b[0], b[1], b[2]); | ||
| } | ||
|
|
||
| function g_slice(uint[] calldata a) public returns(uint, uint, uint, uint) { | ||
| (uint[] calldata b, ) = true ? (a[:], 0) : (a[0:1], 0); | ||
| return (b.length, b[0], b[1], b[2]); | ||
| } | ||
|
|
||
| function g_mix_array_slice(uint[] calldata a) public returns(uint, uint, uint, uint) { | ||
| (uint[] calldata b, ) = true ? (a, 0) : (a[:], 0); | ||
| return (b.length, b[0], b[1], b[2]); | ||
| } | ||
|
|
||
| function g_static(uint[3] calldata a) public returns(uint, uint, uint, uint) { | ||
| (uint[3] calldata b, ) = true ? (a, 0) : (a, 0); | ||
| return (b.length, b[0], b[1], b[2]); | ||
| } | ||
| } | ||
| // ---- | ||
| // g_int(uint256[]): 0x20, 3, 11111111, 2222222, 888888888 -> 3, 11111111, 2222222, 888888888 | ||
| // g_slice(uint256[]): 0x20, 3, 11111111, 2222222, 888888888 -> 3, 11111111, 2222222, 888888888 | ||
| // g_mix_array_slice(uint256[]): 0x20, 3, 11111111, 2222222, 888888888 -> 3, 11111111, 2222222, 888888888 | ||
| // g_static(uint256[3]): 11111111, 2222222, 888888888 -> 3, 11111111, 2222222, 888888888 |
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add tests for slices and static arrays as well. At least the latter also trigger the ICE. contract C {
function f(uint[] calldata a) public {
(uint[] calldata b, ) = true ? (a[:], 0) : (a[0:1], 0);
}
}contract C {
function f(uint[3] calldata a) public {
(uint[3] calldata b, ) = true ? (a, 0) : (a, 0);
}
}Also a mix of arrays and slices (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBH I'd have no idea what this test does just from the current name. What do you mean by "ordinary types"? This should be way more specific:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,62 @@ | ||||||||||||||||||||||
| pragma abicoder v2; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| contract C { | ||||||||||||||||||||||
| struct N { | ||||||||||||||||||||||
| address addr; | ||||||||||||||||||||||
| bytes data; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| struct S { | ||||||||||||||||||||||
| uint8 a; | ||||||||||||||||||||||
| address addr; | ||||||||||||||||||||||
| N[] ns; | ||||||||||||||||||||||
| bytes data; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function g() public returns(S[] memory) { | ||||||||||||||||||||||
| S[] memory ss = new S[](3); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ss[0].a = 123; | ||||||||||||||||||||||
| ss[0].addr = address(1); | ||||||||||||||||||||||
| ss[0].ns = new N[](1); | ||||||||||||||||||||||
| ss[0].ns[0].addr = address(1); | ||||||||||||||||||||||
| ss[0].ns[0].data = "abdeff00"; | ||||||||||||||||||||||
| ss[0].data = "abdeff"; | ||||||||||||||||||||||
|
Comment on lines
21
to
24
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ss[1].a = 124; | ||||||||||||||||||||||
| ss[1].addr = address(2); | ||||||||||||||||||||||
| ss[1].ns = new N[](2); | ||||||||||||||||||||||
| ss[1].ns[0].addr = address(2); | ||||||||||||||||||||||
| ss[1].ns[0].data = "abdeff10"; | ||||||||||||||||||||||
| ss[1].ns[1].addr = address(2); | ||||||||||||||||||||||
| ss[1].ns[1].data = "abdeff11"; | ||||||||||||||||||||||
| ss[1].data = "deabff"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| ss[2].a = 125; | ||||||||||||||||||||||
| ss[2].addr = address(3); | ||||||||||||||||||||||
| ss[2].ns = new N[](3); | ||||||||||||||||||||||
| ss[2].ns[0].addr = address(3); | ||||||||||||||||||||||
| ss[2].ns[0].data = "abdeff20"; | ||||||||||||||||||||||
| ss[2].ns[1].addr = address(3); | ||||||||||||||||||||||
| ss[2].ns[1].data = "abdeff21"; | ||||||||||||||||||||||
| ss[2].ns[2].addr = address(3); | ||||||||||||||||||||||
| ss[2].ns[2].data = "abdeff22"; | ||||||||||||||||||||||
| ss[2].data = "deffab"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return ss; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| function g(S[] calldata a) public returns(S[] memory) { | ||||||||||||||||||||||
| (S[] calldata b, ) = true ? (a, 0) : (a, 0); | ||||||||||||||||||||||
| return b; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| // Result of g(), result and input of the last call must all be equal. | ||||||||||||||||||||||
| // ---- | ||||||||||||||||||||||
| // g() | ||||||||||||||||||||||
| // -> | ||||||||||||||||||||||
| // 0x20, 0x03, 0x60, 0x01e0, 0x0400, 123, 1, 0x80, 0x0140, 0x01, 0x20, 1, 0x40, 8, "abdeff00", 6, "abdeff", 124, 2, 0x80, 0x01e0, 2, 0x40, 0xc0, 2, 0x40, 0x08, "abdeff10", 0x02, 0x40, 0x08, "abdeff11", 6, "deabff", 125, 3, 0x80, 0x0280, 3, 0x60, 0xe0, 0x0160, 3, 0x40, 0x08, "abdeff20", 0x03, 0x40, 0x08, "abdeff21", 0x03, 0x40, 0x08, "abdeff22", 6, "deffab" | ||||||||||||||||||||||
| // g((uint8,address,(address,bytes)[],bytes)[]): | ||||||||||||||||||||||
| // 0x20, 0x03, 0x60, 0x01e0, 0x0400, 123, 1, 0x80, 0x0140, 0x01, 0x20, 1, 0x40, 8, "abdeff00", 6, "abdeff", 124, 2, 0x80, 0x01e0, 2, 0x40, 0xc0, 2, 0x40, 0x08, "abdeff10", 0x02, 0x40, 0x08, "abdeff11", 6, "deabff", 125, 3, 0x80, 0x0280, 3, 0x60, 0xe0, 0x0160, 3, 0x40, 0x08, "abdeff20", 0x03, 0x40, 0x08, "abdeff21", 0x03, 0x40, 0x08, "abdeff22", 6, "deffab" | ||||||||||||||||||||||
| // -> | ||||||||||||||||||||||
| // 0x20, 0x03, 0x60, 0x01e0, 0x0400, 123, 1, 0x80, 0x0140, 0x01, 0x20, 1, 0x40, 8, "abdeff00", 6, "abdeff", 124, 2, 0x80, 0x01e0, 2, 0x40, 0xc0, 2, 0x40, 0x08, "abdeff10", 0x02, 0x40, 0x08, "abdeff11", 6, "deabff", 125, 3, 0x80, 0x0280, 3, 0x60, 0xe0, 0x0160, 3, 0x40, 0x08, "abdeff20", 0x03, 0x40, 0x08, "abdeff21", 0x03, 0x40, 0x08, "abdeff22", 6, "deffab" | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check #16066 (comment)? We should really check why something like
true ? (a, true) : (a, true)does not trigger this bug. This may mean that either we're not doing an array conversion in some case were we need it or that we shouldn't be doing it in thetrue ? (a, 0) : (a, 0)case.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For string and integer literals the conversion function is generated, because the type categories for tuple elements are different. We are assigning
StringLiteraltoArray(string). To be decided if we should change it.For
uint[] memoryit was a bug in the implementation of tuple type comparison. It just compares vectors of pointers. I already fixed it.