[Sol2Yul] Fixed an ICE on struct member copy #12579

hrkrshnn · 2022-01-24T10:57:58Z

Closes #12220
Closes #12558

ekpyron

Mainly needs a changelog entry.

hrkrshnn · 2022-01-25T13:53:49Z

@ekpyron Does it need a changelog entry? It's a change in Yul.

ekpyron · 2022-01-25T13:55:35Z

@ekpyron Does it need a changelog entry? It's a change in Yul.

Yes, but those have been added to the changelog for a while now.

ekpyron · 2022-01-25T16:53:21Z

So, can you add the entry ;-)?

Changelog.md

ekpyron · 2022-01-26T12:47:25Z

Changelog.md

@@ -17,6 +17,7 @@ Bugfixes:
 * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis.
 * Immutables: Fix wrong error when the constructor of a base contract uses ``return`` and the parent contract contains immutable variables.
 * IR Generator: Fix IR syntax error when copying storage arrays of structs containing functions.
+ * IR Generator: Fix IR syntax error when copying reference types in calldata and storage to struct members in memory.


It's not only struct members, but also array elements, isn't it?
Might actually be worth another simple test case.

Actually yeah, also array members, the test test/libsolidity/semanticTests/structs/copy_from_calldata.sol shows it.

We could add a few more tests, i.e. copying storage arrays to a memory array element, etc.... we can also consider that part of #12559, but we can also do it while we're at it...

hrkrshnn · 2022-01-26T12:50:38Z

Changelog.md

@@ -17,6 +17,7 @@ Bugfixes:
 * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis.
 * Immutables: Fix wrong error when the constructor of a base contract uses ``return`` and the parent contract contains immutable variables.
 * IR Generator: Fix IR syntax error when copying storage arrays of structs containing functions.
+ * IR Generator: Fix IR syntax error when copying reference types in calldata and storage to struct members in memory.


Suggested change

* IR Generator: Fix IR syntax error when copying reference types in calldata and storage to struct members in memory.

* IR Generator: Fix IR syntax error when copying reference types in calldata and storage to struct or array members in memory.

@ekpyron would that work? I can commit and squash it afterwards.

Hmm. Github is not rendering it properly! I'll add "or array" members.

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

bshastry

Looks good to me.

ekpyron · 2022-01-27T11:26:23Z

Changelog still needs to include the array part.

hrkrshnn · 2022-01-27T12:02:11Z

@ekpyron sorry, will do this today.

ekpyron · 2022-01-31T11:36:56Z

Still missing the changelog entry change ;-).

hrkrshnn · 2022-01-31T12:52:51Z

test/libsolidity/semanticTests/structs/copy_struct_array_from_storage.sol

@@ -0,0 +1,96 @@
+pragma abicoder v2;
+


Added this example: @ekpyron

But this one surprisingly also compiles in other versions. So independent of the fix.

Hm... I'm pretty sure that there were cases that didn't work in other versions... let me see...

contract C { uint256[] x; function f() public { uint256[][] memory y = new uint256[][](1); y[0] = x; } }

crashes on 0.8.11 with --experimental-via-ir --bin for me...

Actually, this test case you added also does, so it's fine - but it's not independent of the fix :-).

Ah, my bad. Forgot to add --ir flag to the test. So yeah, it does crash!

ekpyron

Code change and test look good, the changelog entry is still not really accurate, but I'm already approving.

ekpyron · 2022-01-31T13:07:22Z

Changelog.md

@@ -17,6 +17,7 @@ Bugfixes:
 * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis.
 * Immutables: Fix wrong error when the constructor of a base contract uses ``return`` and the parent contract contains immutable variables.
 * IR Generator: Fix IR syntax error when copying storage arrays of structs containing functions.
+ * IR Generator: Fix IR syntax error when copying reference types in calldata and storage to struct or array members in memory.


Actually it wasn't really an IR syntax error, but a failing assertion, resp. it was unimplemented so far...

Fixing this now.

hrkrshnn · 2022-01-31T13:09:55Z

Changelog.md

@@ -17,6 +17,7 @@ Bugfixes:
 * Control Flow Graph: Perform proper virtual lookup for modifiers for uninitialized variable and unreachable code analysis.
 * Immutables: Fix wrong error when the constructor of a base contract uses ``return`` and the parent contract contains immutable variables.
 * IR Generator: Fix IR syntax error when copying storage arrays of structs containing functions.
+ * IR Generator: Fix ICE when copying reference types in calldata and storage to struct or array members in memory.


@ekpyron Changed to ICE.

Actually, now it isn't sorted alphabetically anymore ;-D.

ekpyron · 2022-01-31T13:37:07Z

It has a merge conflict now and something weird happened to CI (but looks like a weird CirecleCI issue independent of the PR) - so I'd hope a rebase will fix it...

hrkrshnn · 2022-01-31T13:51:07Z

Rebased @ekpyron :)

chriseth · 2022-01-31T16:09:26Z

Changelog.md

@@ -18,6 +18,7 @@ Bugfixes:
 * Immutables: Fix wrong error when the constructor of a base contract uses ``return`` and the parent contract contains immutable variables.
 * IR Generator: Add missing cleanup during the conversion of fixed bytes types to smaller fixed bytes types.
 * IR Generator: Add missing cleanup for indexed event arguments of value type.
+ * IR Generator: Fix ICE when copying reference types in calldata and storage to struct or array members in memory.


We use ICE liberally, but the term is really difficult to understand without background and "internal error" is almost as short.

I can fix this. There are liked 4 other ICEs in the same section! Will leave them untouched in this PR.

hrkrshnn force-pushed the fix-memory-copy-bug branch 2 times, most recently from aa99de4 to c3c5aac Compare January 24, 2022 11:46

hrkrshnn marked this pull request as ready for review January 24, 2022 11:47

hrkrshnn requested review from ekpyron, cameel and bshastry January 24, 2022 11:47

hrkrshnn changed the title ~~Fixed a ICE on calldata to struct member copy~~ Fixed an ICE on struct member copy Jan 24, 2022

hrkrshnn force-pushed the fix-memory-copy-bug branch 2 times, most recently from c881e8f to a82eef9 Compare January 24, 2022 12:33

hrkrshnn changed the title ~~Fixed an ICE on struct member copy~~ [Sol2Yul] Fixed an ICE on struct member copy Jan 24, 2022

ekpyron reviewed Jan 25, 2022

View reviewed changes

hrkrshnn force-pushed the fix-memory-copy-bug branch 2 times, most recently from e485450 to b324ccc Compare January 26, 2022 12:14

ekpyron reviewed Jan 26, 2022

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

hrkrshnn force-pushed the fix-memory-copy-bug branch from b324ccc to 32ed69c Compare January 26, 2022 12:46

ekpyron reviewed Jan 26, 2022

View reviewed changes

hrkrshnn commented Jan 26, 2022

View reviewed changes

bshastry reviewed Jan 26, 2022

View reviewed changes

libsolidity/codegen/ir/IRGeneratorForStatements.cpp Show resolved Hide resolved

bshastry previously approved these changes Jan 26, 2022

View reviewed changes

hrkrshnn dismissed bshastry’s stale review via 7802a6b January 31, 2022 12:51

hrkrshnn force-pushed the fix-memory-copy-bug branch from 32ed69c to 7802a6b Compare January 31, 2022 12:51

hrkrshnn commented Jan 31, 2022

View reviewed changes

hrkrshnn requested a review from ekpyron January 31, 2022 12:54

ekpyron previously approved these changes Jan 31, 2022

View reviewed changes

hrkrshnn dismissed ekpyron’s stale review via 083090b January 31, 2022 13:09

hrkrshnn force-pushed the fix-memory-copy-bug branch from 7802a6b to 083090b Compare January 31, 2022 13:09

hrkrshnn commented Jan 31, 2022

View reviewed changes

ekpyron previously approved these changes Jan 31, 2022

View reviewed changes

hrkrshnn dismissed ekpyron’s stale review via 0e27752 January 31, 2022 13:33

hrkrshnn force-pushed the fix-memory-copy-bug branch from 083090b to 0e27752 Compare January 31, 2022 13:33

This comment has been minimized.

Sign in to view

hrkrshnn force-pushed the fix-memory-copy-bug branch 2 times, most recently from 3f400d1 to 58f8e52 Compare January 31, 2022 13:50

ekpyron previously approved these changes Jan 31, 2022

View reviewed changes

chriseth reviewed Jan 31, 2022

View reviewed changes

Fixed a ICE on calldata to struct member copy

0fe5811

hrkrshnn dismissed ekpyron’s stale review via 0fe5811 January 31, 2022 16:26

hrkrshnn force-pushed the fix-memory-copy-bug branch from 58f8e52 to 0fe5811 Compare January 31, 2022 16:26

ekpyron approved these changes Jan 31, 2022

View reviewed changes

chriseth merged commit fd68cb1 into develop Jan 31, 2022

chriseth deleted the fix-memory-copy-bug branch January 31, 2022 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sol2Yul] Fixed an ICE on struct member copy #12579

[Sol2Yul] Fixed an ICE on struct member copy #12579

hrkrshnn commented Jan 24, 2022 •

edited

ekpyron left a comment

hrkrshnn commented Jan 25, 2022

ekpyron commented Jan 25, 2022 •

edited

ekpyron commented Jan 25, 2022

ekpyron Jan 26, 2022 •

edited

hrkrshnn Jan 26, 2022

ekpyron Jan 26, 2022

hrkrshnn Jan 26, 2022

hrkrshnn Jan 26, 2022 •

edited

bshastry left a comment

ekpyron commented Jan 27, 2022

hrkrshnn commented Jan 27, 2022

ekpyron commented Jan 31, 2022

hrkrshnn Jan 31, 2022

hrkrshnn Jan 31, 2022

ekpyron Jan 31, 2022

ekpyron Jan 31, 2022

ekpyron Jan 31, 2022

hrkrshnn Jan 31, 2022

ekpyron left a comment

ekpyron Jan 31, 2022

hrkrshnn Jan 31, 2022

hrkrshnn Jan 31, 2022

ekpyron Jan 31, 2022

hrkrshnn Jan 31, 2022

ekpyron commented Jan 31, 2022

This comment has been minimized.

hrkrshnn commented Jan 31, 2022

chriseth Jan 31, 2022 •

edited

hrkrshnn Jan 31, 2022 •

edited

hrkrshnn Jan 31, 2022

	* IR Generator: Fix IR syntax error when copying reference types in calldata and storage to struct members in memory.
	* IR Generator: Fix IR syntax error when copying reference types in calldata and storage to struct or array members in memory.

[Sol2Yul] Fixed an ICE on struct member copy #12579

[Sol2Yul] Fixed an ICE on struct member copy #12579

Conversation

hrkrshnn commented Jan 24, 2022 • edited

ekpyron left a comment

Choose a reason for hiding this comment

hrkrshnn commented Jan 25, 2022

ekpyron commented Jan 25, 2022 • edited

ekpyron commented Jan 25, 2022

ekpyron Jan 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn Jan 26, 2022 • edited

Choose a reason for hiding this comment

bshastry left a comment

Choose a reason for hiding this comment

ekpyron commented Jan 27, 2022

hrkrshnn commented Jan 27, 2022

ekpyron commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekpyron commented Jan 31, 2022

This comment has been minimized.

hrkrshnn commented Jan 31, 2022

chriseth Jan 31, 2022 • edited

Choose a reason for hiding this comment

hrkrshnn Jan 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrkrshnn commented Jan 24, 2022 •

edited

ekpyron commented Jan 25, 2022 •

edited

ekpyron Jan 26, 2022 •

edited

hrkrshnn Jan 26, 2022 •

edited

chriseth Jan 31, 2022 •

edited

hrkrshnn Jan 31, 2022 •

edited