Skip to content

External calls returning only value types no longer allocate persistent memory in IR and legacy pipelines #16444

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.

Sign up for GitHub

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

Draft
r0qs wants to merge 2 commits into
base: develop
Choose a base branch
from
+535 −651
Draft

External calls returning only value types no longer allocate persistent memory in IR and legacy pipelines #16444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bb4bb1
via-ir: Skip finalizeAllocation for external calls returning only val…
r0qs Feb 4, 2026
05c819d
evmasm: Don't update free memory pointer for value type returns
r0qs Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions libsolidity/codegen/ExpressionCompiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2942,11 +2942,10 @@ void ExpressionCompiler::appendExternalFunctionCall(
utils().fetchFreeMemoryPointer();
// Stack: return_data_start

// The old decoder did not allocate any memory (i.e. did not touch the free
// memory pointer), but kept references to the return data for
// (statically-sized) arrays
Comment on lines -2945 to -2947
Copy link
Collaborator

@cameel cameel Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you removing this? Isn't it still true? I don't see any allocation for static types in CompilerUtils::abiDecode().

Though I also do not see any decoding for such types there, but I guess it's because it does not support nested arrays and assumes that for other types the memory layout matches calldata layout? What about arrays of structs though?

Also, if that's the case then the logic checking ReferenceType below is also wrong...

// Only update free memory pointer if any return type needs memory (reference types).
// Value types are decoded directly to the stack via mload.
bool needToUpdateFreeMemoryPtr = false;
if (dynamicReturnSize || m_context.useABICoderV2())
if (dynamicReturnSize)
needToUpdateFreeMemoryPtr = true;
else
for (auto const& retType: returnTypes)
Expand Down
16 changes: 14 additions & 2 deletions libsolidity/codegen/ir/IRGeneratorForStatements.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include <libsolutil/Visitor.h>

#include <range/v3/algorithm/all_of.hpp>
#include <range/v3/algorithm/any_of.hpp>
#include <range/v3/view/transform.hpp>

using namespace solidity;
Expand Down Expand Up @@ -2704,8 +2705,10 @@ void IRGeneratorForStatements::appendExternalFunctionCall(
</supportsReturnData>
</isReturndataSizeDynamic>

// update freeMemoryPointer according to dynamic return size
<finalizeAllocation>(<pos>, <returnDataSizeVar>)
<?needToUpdateFreeMemoryPtr>
// update freeMemoryPointer according to dynamic return size
<finalizeAllocation>(<pos>, <returnDataSizeVar>)
</needToUpdateFreeMemoryPtr>

// decode return parameters from external try-call into retVars
<?+retVars> <retVars> := </+retVars> <abiDecode>(<pos>, add(<pos>, <returnDataSizeVar>))
Expand Down Expand Up @@ -2738,6 +2741,15 @@ void IRGeneratorForStatements::appendExternalFunctionCall(
templ("success", m_context.newYulVariable());
templ("allocateUnbounded", m_utils.allocateUnboundedFunction());
templ("finalizeAllocation", m_utils.finalizeAllocationFunction());

// Only update free memory pointer if any return type needs memory (reference types).
// Value types are decoded directly to the stack via mload.
Copy link
Collaborator

@cameel cameel Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this may be true now, I think that just straight out hard-coding this assumption here is fragile and can lead to bugs.

The assumption here is actually a bit broader than just that we're only dealing with value types. We can safely free this memory if and only if we know there will be no other memory allocations before we're done using this piece of memory. There is no guarantee that the decoder won't in the future allocate any extra memory for other purposes, even when it's only dealing with value types.

A safer approach would be to modify tupleDecoder() to return a value that tells us whether the function it generated needs any allocations. It would at least clearly signal to anyone modifying it that the downstream code relies on whether memory was allocated. It would also be more thorough - if there are any other cases in which we don't allocate, we'll avoid allocating in those too.

Copy link
Collaborator

@cameel cameel Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, checking tupleDecoder() I noticed that this is not the only case where we can do this optimization. We're doing pretty much the same thing in YulUtilFunctions::copyConstructorArgumentsToMemoryFunction() when we're decoding arguments in a constructor.

bool needToUpdateFreeMemoryPtr = ranges::any_of(
returnInfo.returnTypes,
[](auto const& t) { return !t->decodingType()->isValueType(); }
);
templ("needToUpdateFreeMemoryPtr", needToUpdateFreeMemoryPtr);

templ("shl28", m_utils.shiftLeftFunction(8 * (32 - 4)));

templ("funSel", IRVariable(_functionCall.expression()).part("functionSelector").name());
Expand Down
104 changes: 22 additions & 82 deletions test/cmdlineTests/standard_debug_info_in_evm_asm_via_ir_location/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ sub_0: assembly {
dup1
revert
tag_8:
jumpi(tag_26, callvalue)
jumpi(tag_26, slt(add(not(0x03), calldatasize), 0x00))
jumpi(tag_24, callvalue)
jumpi(tag_24, slt(add(not(0x03), calldatasize), 0x00))
sload(0x00)
sub(shl(0xff, 0x01), 0x01)
dup2
Expand Down Expand Up @@ -175,40 +175,17 @@ sub_0: assembly {
swap2
pop
0x20
dup3
jumpi(tag_22, gt(0x20, returndatasize))
tag_23:
/* \"C\":79:503 contract C... */
0x1f
dup2
add
not(0x1f)
and
dup4
add
0xffffffffffffffff
dup2
gt
dup5
dup3
lt
or
tag_24
jumpi
/* \"C\":475:483 this.f() */
0x20
/* \"C\":79:503 contract C... */
swap2
dup5
swap2
0x40
mstore
/* \"C\":475:483 this.f() */
dup2
add
/* \"C\":79:503 contract C... */
sub
slt
tag_26
tag_24
jumpi
swap1
mload
Expand All @@ -218,22 +195,15 @@ sub_0: assembly {
/* \"C\":475:483 this.f() */
jump(tag_19)
/* \"C\":79:503 contract C... */
tag_26:
tag_24:
0x00
dup1
revert
tag_24:
/* \"C\":120:122 41 */
shl(0xe0, 0x4e487b71)
/* \"C\":79:503 contract C... */
0x00
mstore
mstore(0x04, 0x41)
revert(0x00, 0x24)
/* \"C\":475:483 this.f() */
tag_22:
pop
returndatasize
swap2
pop
jump(tag_23)
tag_16:
/* \"C\":79:503 contract C... */
Expand Down Expand Up @@ -264,8 +234,8 @@ sub_0: assembly {
revert
/* \"C\":79:503 contract C... */
tag_6:
jumpi(tag_26, callvalue)
jumpi(tag_26, slt(add(not(0x03), calldatasize), 0x00))
jumpi(tag_24, callvalue)
jumpi(tag_24, slt(add(not(0x03), calldatasize), 0x00))
0x20
sload(0x00)
mload(0x40)
Expand All @@ -274,8 +244,8 @@ sub_0: assembly {
mstore
return
tag_4:
jumpi(tag_26, callvalue)
jumpi(tag_26, slt(add(not(0x03), calldatasize), 0x00))
jumpi(tag_24, callvalue)
jumpi(tag_24, slt(add(not(0x03), calldatasize), 0x00))
/* \"C\":326:334 immutVar */
immutable(\"0xe4b1702d9298fee62dfeccc57d322a463ad55ca201256d01f62b45b2e1c21c10\")
/* \"C\":120:122 41 */
Expand Down Expand Up @@ -447,8 +417,8 @@ sub_0: assembly {
dup1
revert
tag_8:
jumpi(tag_26, callvalue)
jumpi(tag_26, slt(add(not(0x03), calldatasize), 0x00))
jumpi(tag_24, callvalue)
jumpi(tag_24, slt(add(not(0x03), calldatasize), 0x00))
sload(0x00)
sub(shl(0xff, 0x01), 0x01)
dup2
Expand Down Expand Up @@ -521,40 +491,17 @@ sub_0: assembly {
swap2
pop
0x20
dup3
jumpi(tag_22, gt(0x20, returndatasize))
tag_23:
/* \"D\":91:181 contract D is C(3)... */
0x1f
dup2
add
not(0x1f)
and
dup4
add
0xffffffffffffffff
dup2
gt
dup5
dup3
lt
or
tag_24
jumpi
/* \"C\":475:483 this.f() */
0x20
/* \"D\":91:181 contract D is C(3)... */
swap2
dup5
swap2
0x40
mstore
/* \"C\":475:483 this.f() */
dup2
add
/* \"D\":91:181 contract D is C(3)... */
sub
slt
tag_26
tag_24
jumpi
swap1
mload
Expand All @@ -564,22 +511,15 @@ sub_0: assembly {
/* \"C\":475:483 this.f() */
jump(tag_19)
/* \"D\":91:181 contract D is C(3)... */
tag_26:
tag_24:
0x00
dup1
revert
tag_24:
/* \"C\":120:122 41 */
shl(0xe0, 0x4e487b71)
/* \"D\":91:181 contract D is C(3)... */
0x00
mstore
mstore(0x04, 0x41)
revert(0x00, 0x24)
/* \"C\":475:483 this.f() */
tag_22:
pop
returndatasize
swap2
pop
jump(tag_23)
tag_16:
/* \"D\":91:181 contract D is C(3)... */
Expand Down Expand Up @@ -610,8 +550,8 @@ sub_0: assembly {
revert
/* \"D\":91:181 contract D is C(3)... */
tag_6:
jumpi(tag_26, callvalue)
jumpi(tag_26, slt(add(not(0x03), calldatasize), 0x00))
jumpi(tag_24, callvalue)
jumpi(tag_24, slt(add(not(0x03), calldatasize), 0x00))
0x20
sload(0x00)
mload(0x40)
Expand All @@ -620,8 +560,8 @@ sub_0: assembly {
mstore
return
tag_4:
jumpi(tag_26, callvalue)
jumpi(tag_26, slt(add(not(0x03), calldatasize), 0x00))
jumpi(tag_24, callvalue)
jumpi(tag_24, slt(add(not(0x03), calldatasize), 0x00))
/* \"C\":326:334 immutVar */
immutable(\"0xe4b1702d9298fee62dfeccc57d322a463ad55ca201256d01f62b45b2e1c21c10\")
/* \"C\":120:122 41 */
Expand Down
22 changes: 0 additions & 22 deletions test/cmdlineTests/standard_debug_info_in_yul_location/output.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,9 +569,6 @@ object \"C_54\" {
_13 := returndatasize()
}

// update freeMemoryPointer according to dynamic return size
finalize_allocation(_10, _13)

// decode return parameters from external try-call into retVars
expr_47 := abi_decode_tuple_t_int256_fromMemory(_10, add(_10, _13))
}
Expand Down Expand Up @@ -714,14 +711,6 @@ object \"C_54\" {
let _6 := 32
if gt(32, returndatasize()) { _6 := returndatasize() }
/// @src 0:79:510 \"contract C...\"
let newFreePtr := add(_4, and(add(_6, 31), not(31)))
if or(gt(newFreePtr, 0xffffffffffffffff), lt(newFreePtr, _4))
{
mstore(0, shl(224, 0x4e487b71))
mstore(4, 0x41)
revert(0, 0x24)
}
mstore(64, newFreePtr)
Comment on lines -717 to -724
Copy link
Collaborator

@cameel cameel Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised how much code this simple change removes. Looks like finalize_allocation() and its panic_error_0x41() are getting inlined quite a lot, which bloats the bytecode.

if slt(sub(/** @src 0:482:490 \"this.f()\" */ add(_4, _6), /** @src 0:79:510 \"contract C...\" */ _4), /** @src 0:482:490 \"this.f()\" */ 32)
/// @src 0:79:510 \"contract C...\"
{ revert(0, 0) }
Expand Down Expand Up @@ -1406,9 +1395,6 @@ object \"D_72\" {
_13 := returndatasize()
}

// update freeMemoryPointer according to dynamic return size
finalize_allocation(_10, _13)

// decode return parameters from external try-call into retVars
expr_47 := abi_decode_tuple_t_int256_fromMemory(_10, add(_10, _13))
}
Expand Down Expand Up @@ -1558,14 +1544,6 @@ object \"D_72\" {
let _6 := 32
if gt(32, returndatasize()) { _6 := returndatasize() }
/// @src 1:91:181 \"contract D is C(3)...\"
let newFreePtr := add(_4, and(add(_6, 31), not(31)))
if or(gt(newFreePtr, 0xffffffffffffffff), lt(newFreePtr, _4))
{
mstore(0, shl(224, 0x4e487b71))
mstore(4, 0x41)
revert(0, 0x24)
}
mstore(64, newFreePtr)
if slt(sub(/** @src 0:482:490 \"this.f()\" */ add(_4, _6), /** @src 1:91:181 \"contract D is C(3)...\" */ _4), /** @src 0:482:490 \"this.f()\" */ 32)
/// @src 1:91:181 \"contract D is C(3)...\"
{ revert(0, 0) }
Expand Down
Loading