Fix optimising out `returndatacopy` call. by rodiazet · Pull Request #16436 · argotorg/solidity

rodiazet · 2026-02-02T21:51:44Z

Fixes removing returndatacopy calls when returndatasize is stale.

This problem is fixed by registering returndatasize calls and invalidating them when any of call opcode (potentially changing the size) is called.

cameel · 2026-02-03T14:31:21Z

WiP. Need more tests.

And a changelog entry. And a buglist entry. Generally an integral part of the task is to determine the actual impact of the bug on the compiler and do a deeper validation of the claims from the submission (which we have confirmed but only superficially investigated so far).

cameel · 2026-02-20T21:13:09Z

@rodiazet Since this one seems simpler to fix than #16459 and was reported earlier I think it would make sense to focus on finishing it first. I'd like to get it into the next release if possible.

rodiazet · 2026-02-23T16:04:46Z

Actually this one can be fixed also by improving SSAValueTracker, to make it aware of the fact that calls invalidate returndatasize(), but it would require also registering all the returndatasize() calls with all variables which depends on them, directly and indirectly, which probably would be inefficient.

nikola-matic · 2026-02-24T11:59:21Z

libyul/optimiser/UnusedStoreEliminator.cpp

+		registerReturnDataSizeCall(_functionCall);
+		invalidateReturnDataSize(_functionCall);


Just shove this into one function and call it validateReturnDataSizeCall(...); it looks very odd at first glance to register and then immediately after invalidate.

Right. Initially I thought they gonna be called in different places.

nikola-matic · 2026-02-24T12:01:30Z

Changelog.md

 Compiler Features:

 Bugfixes:
+ * Yul Optimizer: Fix ``UnusedStoreEliminator`` incorrectly removing ``returndatacopy`` operations when the length comes from a stale ``returndatasize()`` call that was invalidated by subsequent call opcodes.


Suggested change

* Yul Optimizer: Fix ``UnusedStoreEliminator`` incorrectly removing ``returndatacopy`` operations when the length comes from a stale ``returndatasize()`` call that was invalidated by subsequent call opcodes.

* Yul Optimizer: Fix `UnusedStoreEliminator` incorrectly removing `returndatacopy` operations when the length comes from a stale `returndatasize()` call that was invalidated by subsequent call opcodes.

I don't commit suggestions from you any longer.

nikola-matic · 2026-02-24T12:19:34Z

libyul/optimiser/UnusedStoreEliminator.h

+
+	// Contains valid return data size calls. Call is valid until any of invalidating return data size instruction is
+	// called. Currently, they are CALL, STATICCALL or DELEGATECALL.
+	std::set<FunctionCall const*> m_validReturnDataSizeCalls;


This could be a boolean.

I think it cannot be a boolean. We need to track which returndatasize calls are still valid. Only these are valid which are called after last invalidating call.

If they're visited in sequential order, then a boolean will be perfectly fine. @cameel what's the visitation order here?

Someone can produce code like this or similar in inline assembly. Or an optimiser step can create it.

let s := returndatasize(); staticcall(...) let s1 := returndatasize(); returndatacopy(..., s) // This cannot be removed returndatacopy(..., s1) // This can be removed.

Yeah, the fact that visitation order is linear does not help, because you can refer to older calls that are no longer valid.

But I'm not sure the current solution is even enough. How does it account for loops and branching?

let s := returndatasize() for { let i := 0 } lt(i, 10) { i := add(i, 1) } { returndatacopy(..., s) staticcall(...) }

Here s is valid but only in the first iteration of the loop. Which means that we cannot remove the returndatacopy(), but a linear scan not accounting for loops will miss that.

BTW. Someone wrote that this issue looks simpler....... :)

Consider yourself lucky Daniel isn't here, he would have called it trivial.

optimizing our returndatacopy in unused store eliminator is a bit weird anyway. It was likely the most convenient place to add such an optimization and then it stuck.

No, this is fully within the its scope:

Optimiser component that removes stores to memory and storage slots that are not used or overwritten later on.

Store does not specify whether this is about memory or storage. Storing the result in memory is inherently a part of the call operation.

I agree with @rodiazet that a dedicated Yul builtin would be the most elegant solution to the general problem. We could basically fix the underlying design choice by having the builtin either always copy all returndata or cap the length to no more than returndatasize(). But I also think that this goes beyond the scope of a mere fix and I would not do that here. Feel free to report an issue for it though, it's worth considering as a feature.

@nikola-matic has a point that the current solution is brittle and it would better not to have to do this. We're risking introducing another bug if we don't properly account for all cases and that's tricky. My suggestion would be to try removing this optimization completely (in a new PR) and see how much effect that has. I suspect the optimization is actually largely ineffective and we don't need it.

I checked the code and found only a handful of situations where codegen is using returndatacopy():

Explicit use in inline assembly.

Copying the result of external calls that return dynamic types (via IRGeneratorForStatements::appendExternalFunctionCall).

Bare external calls (via YulUtilFunctions::extractReturndataFunction())

Checking bits of returndata in try/catch (selector, panic code, etc.)

These don't even match this pattern because they either hard-code the length or use an expression more complex than just returndatasize(). See returnDataSelectorFunction(), tryDecodeErrorMessageFunction() and tryDecodePanicDataFunction() in YulUtilFunctions.

Forwarding reverts.

In this case returndata is always used so the optimization does not apply.

The corresponding cases in evmasm codegen.

These don't go through Yul optimizer.

EOF precompile calls.

These use hard-coded length.

Of the above, only 1-3 can really trigger the optimization. The problem is that in practice returndatacopy() for external calls is likely never removed anyway, because without memory objects we are unable to properly determine that the copied data is unused.

For example, if you compile this, you can still see returndatacopy() in the optimized IR output:

contract C { function f() external returns (uint[] memory) {} function testExternalCall() public { this.f(); } function testBareCall() public { address(this).call(""); } }

Relevant part of testBareCall():

case 0x3ae2d4f4 { // ... data := memPtr returndatacopy(add(memPtr, 0x20), 0, returndatasize()) } return(0, 0) }

Relevant part of testExternalCall():

case 0x82168021 { // ... let _5 := returndatasize() returndatacopy(_3, 0, _5) finalize_allocation(_3, _5) // ... let _7 := add(_3, offset) // ... let src := add(_7, 32) for { } lt(src, srcEnd) { src := add(src, 32) } { mstore(dst, mload(src)) dst := add(dst, 32) } } return(0, 0) }

This means that this optimization does not really work for anything other than hand-crafted inline assembly. The comment in the implementation explicitly says that it was not aimed at that use case though.

Removing it will probably not change anything substantial. Our short-term plan for external calls is to optimize memory use at codegen level anyway. We will detect that the result is unused and not generate returndatacopy() in the first place.

OK. Agree on removing this optimisation piece. Pushed the PR and the issue is already created.

cameel · 2026-03-05T10:38:36Z

test/libyul/YulOptimizerTestCommon.cpp

 			ExpressionJoiner::run(*m_context, block);
 			return block;
 		}},
+		{"unusedStoreEliminatorNoSsaTransform", [&]() {


Suggested change

{"unusedStoreEliminatorNoSsaTransform", [&]() {

{"unusedStoreEliminatorNoSSA", [&]() {

cameel · 2026-03-05T10:39:28Z

test/libyul/YulOptimizerTestCommon.cpp

+			ForLoopInitRewriter::run(*m_context, block);
+			ExpressionSplitter::run(*m_context, block);
+			UnusedStoreEliminator::run(*m_context, block);
+			SSAReverser::run(*m_context, block);


If you're not running SSATransform, you don't need to use SSAReverser to get it back to non-SSA either.

cameel · 2026-03-05T11:03:07Z

libyul/optimiser/UnusedStoreEliminator.cpp

+		if (
+			*instruction == evmasm::Instruction::STATICCALL ||
+			*instruction == evmasm::Instruction::CALL ||
+			*instruction == evmasm::Instruction::DELEGATECALL ||
+			*instruction == evmasm::Instruction::CALLCODE
+		)


This is something that will need to be kept up to date as new EVM instructions are added (for example EOF would definitely have affected this). We should define a function for it in SemanticInformation.h. I'd call it invalidatesReturndata().

I think it's also likely that we already have such a check hard-coded somewhere in the compiler. Please check. If we do, those other places should use the new helper too.

github-actions · 2026-03-24T12:18:16Z

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

github-actions · 2026-03-31T12:21:04Z

This pull request was closed due to a lack of activity for 7 days after it was stale.

nikola-matic · 2026-03-31T12:23:21Z

@rodiazet this can stay closed right - we're getting rid of the returndatacopy optimization completely here, right?

rodiazet · 2026-04-01T16:46:47Z

Yes. I wanted to close it after the removal PR is merged, but if it's closed already it does not make sense to reopen it.

cameel added the codegen error Compiler generates invalid code. Critical. label Feb 3, 2026

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Feb 18, 2026

github-actions bot removed the stale The issue/PR was marked as stale because it has been open for too long. label Feb 21, 2026

rodiazet force-pushed the fix-opt-returndatacopy branch 3 times, most recently from 14a99af to 8060165 Compare February 24, 2026 10:30

rodiazet marked this pull request as ready for review February 24, 2026 10:31

nikola-matic reviewed Feb 24, 2026

View reviewed changes

rodiazet added 5 commits February 25, 2026 13:44

Add test cases for returndatacopy optimization with stale returndatasize

48d64f5

Fix optimising out returndatacopy call with stale returndatasize

b9ed714

Add changelog and buglist entry for UnusedStoreEliminator fix

4330bef

Address PR review comments.

34351a9

Address PR review comments.

477266c

rodiazet force-pushed the fix-opt-returndatacopy branch from 4e4e511 to 477266c Compare February 25, 2026 13:01

argotorg deleted a comment from github-actions bot Mar 5, 2026

cameel reviewed Mar 5, 2026

View reviewed changes

This was referenced Mar 9, 2026

Dedicated yul builtin to copy all return data and support it in UnusedStoreEliminator. #16507

Open

Remove RETURNDATACOPY elimination from UnusedStoreEliminator #16508

Open

github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 24, 2026

github-actions bot added the closed-due-inactivity label Mar 31, 2026

github-actions bot closed this Mar 31, 2026

		registerReturnDataSizeCall(_functionCall);
		invalidateReturnDataSize(_functionCall);

	* Yul Optimizer: Fix ``UnusedStoreEliminator`` incorrectly removing ``returndatacopy`` operations when the length comes from a stale ``returndatasize()`` call that was invalidated by subsequent call opcodes.
	* Yul Optimizer: Fix `UnusedStoreEliminator` incorrectly removing `returndatacopy` operations when the length comes from a stale `returndatasize()` call that was invalidated by subsequent call opcodes.

	{"unusedStoreEliminatorNoSsaTransform", [&]() {
	{"unusedStoreEliminatorNoSSA", [&]() {

Conversation

rodiazet commented Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cameel commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cameel commented Feb 20, 2026

Uh oh!

rodiazet commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 24, 2026

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

nikola-matic commented Mar 31, 2026

Uh oh!

rodiazet commented Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rodiazet commented Feb 2, 2026 •

edited

Loading

cameel commented Feb 3, 2026 •

edited

Loading

rodiazet commented Feb 23, 2026 •

edited

Loading

rodiazet Feb 24, 2026 •

edited

Loading

cameel Feb 24, 2026 •

edited

Loading

cameel Mar 5, 2026 •

edited

Loading

rodiazet Mar 9, 2026 •

edited

Loading