Fix incorrect `mstore` removal. by rodiazet · Pull Request #16459 · argotorg/solidity

rodiazet · 2026-02-10T13:23:28Z

SSAValueTracker implementation has changed to support also variables which are functions parameters. Previous implementation omitted function parameters because they do not have default value. SSAValueTracker supported only variables which are initialised, directly in the code or by default value. Functions return variables are set to 0
, but function parameters have values passed by the caller. Additional set is added to store function parameters which are not assigned in the function body. This change allowed to implement function which filters out variables which (with their dependencies) are not in SSA form. It's needed to properly define a set which serves as an input to the knowledge base.

Fixes: #16458

stackenbotten3000 · 2026-02-19T15:59:19Z

There was an error when running chk_coding_style for commit 30c820caf8e6a2c43c096cecf423c67e044284c8:

Coding style error:
libyul/optimiser/SSAValueTracker.cpp:25:#include "liblangutil/Exceptions.h"

Please check that your changes are working as intended.

stackenbotten3000 · 2026-02-19T15:59:19Z

libyul/optimiser/SSAValueTracker.cpp


 #include <libyul/optimiser/SSAValueTracker.h>

+#include "liblangutil/Exceptions.h"


Coding style error

stackenbotten3000 · 2026-02-19T16:23:49Z

There was an error when running chk_coding_style for commit 2a78055a8615e5187958c3c198b257b2d721f8f6:

Coding style error:
libyul/optimiser/SSAValueTracker.cpp:25:#include "liblangutil/Exceptions.h"

Please check that your changes are working as intended.

stackenbotten3000 · 2026-02-19T16:23:49Z

libyul/optimiser/SSAValueTracker.cpp


 #include <libyul/optimiser/SSAValueTracker.h>

+#include "liblangutil/Exceptions.h"


Coding style error

Copilot

Pull request overview

This PR updates the Yul optimizer’s UnusedStoreEliminator pipeline to avoid incorrect mstore removals when the input is not in pseudo-SSA form (notably when function parameters are involved), and adds targeted optimizer tests for the “no SSA transform” scenario described in #16458.

Changes:

Extend SSAValueTracker to treat unassigned function parameters as valid SSA roots when checking SSA-ness transitively through dependencies.
Filter the knowledge-base SSA input used by UnusedStoreEliminator to only include expressions that are in SSA form together with their dependencies.
Add a dedicated unusedStoreEliminatorNoSsaTransform test step and new Yul tests that exercise the problematic cases.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`libyul/optimiser/SSAValueTracker.h`	Adds an SSA-with-dependencies query API and tracks function parameters.
`libyul/optimiser/SSAValueTracker.cpp`	Implements dependency-based SSA checking and parameter tracking during AST walk.
`libyul/optimiser/UnusedStoreEliminator.cpp`	Builds the SSA value map using only SSA-safe expressions (with SSA-safe dependencies).
`test/libyul/YulOptimizerTestCommon.cpp`	Adds a new optimizer test step that runs UnusedStoreEliminator without SSATransform.
`test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/mstore_location_reassigned.yul`	New regression test covering non-SSA reassignment affecting `mstore` location.
`test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_as_mstore_location_unrelated.yul`	New test where parameter-derived store location should be removable (non-overlapping).
`test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_as_mstore_location_covering.yul`	New test where parameter-derived store location must be kept (overlapping return).
`test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_reassinged_as_mstore_location_unrelated.yul`	New test intended to cover reassigned-parameter non-overlap scenario.
`test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_reassinged_as_mstore_location_covering.yul`	New test covering reassigned-parameter overlapping scenario.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libyul/optimiser/SSAValueTracker.h

...StoreEliminatorNoSsaTransform/function_parameter_reassigned_as_mstore_location_unrelated.yul

libyul/optimiser/SSAValueTracker.cpp

nikola-matic

I assumed you squashed the test updates into the first commit (after pushing the fix), because I can't see any updates in the test expectations in the second commit?

As per usual, you're missing a changelog entry :)

libyul/optimiser/SSAValueTracker.cpp

nikola-matic · 2026-02-20T10:18:11Z

libyul/optimiser/SSAValueTracker.cpp

+	else
+	{
+		solAssert(std::holds_alternative<Literal>(*_expression), "Impossible expression type");
+		return true;
+	}


Suggested change

else

{

solAssert(std::holds_alternative<Literal>(*_expression), "Impossible expression type");

return true;

}

solAssert(std::holds_alternative<Literal>(*_expression), "Impossible expression type");

return true;

libyul/optimiser/SSAValueTracker.cpp

nikola-matic · 2026-02-20T11:13:30Z

...Tests/unusedStoreEliminatorNoSsaTransform/function_parameter_as_mstore_location_covering.yul

+//         let a := add(arg, 1)
+//         let b := arg
+//         let outLen := 32
+//         mstore(a, 0xAA)


This worked before the fix as well, since you're not reassigning arg?

Yes, but I added function parameters set to the SSAValueTracker, because I need to know them if they are not reassigned and if not they properly close the closed set. This is just a simple test of this.

...nusedStoreEliminatorNoSsaTransform/transitive_dependency_reassigned_prevents_elimination.yul

...ibyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/multi_return_var_not_tracked.yul

stackenbotten3000 · 2026-02-20T12:53:00Z

There was an error when running chk_coding_style for commit bbdfc8c26e04b7652e55a9615b6d825302381b37:

Error: Trailing whitespace found:
Changelog.md:8:* Yul Optimizer: Fix a bug in `UnusedStoreEliminator`, which could lead to incorrect removal of `mstore`s in certain cases.

Please check that your changes are working as intended.

Changelog.md

cameel · 2026-02-20T20:16:55Z

Changelog.md

Please remember that this will need a buglist entry.

Without it the PR is not really finished so it should still be a draft.

bug list updated. How about the blogpost link? I changed it to draft for now.

You can leave it unfilled until we know when we're releasing this. URLs are predictable, based on date and title (which should match the bug ID) so we could create one already, but if we do it now, we can easily forget to update the date and we'll end up with a broken link. So for now I'd put in something that clearly looks like a placeholder.

I used X/Y/Z placeholder for now as it was done in prev cases.

test.sol

libyul/optimiser/SSAValueTracker.cpp

clonker · 2026-03-02T08:12:04Z

libyul/optimiser/SSAValueTracker.h

Since this is never iterated over in a way where the order would matter (I think, double check this), we could also look at unordered_map or boost::flat_map.

Changed. It's used in 2 different places (FullInliner and the USE) and it looks they do don't depend on the order of the values in the map.

clonker

Two typos in your filenames. Also I think the unusedStoreEliminatorNoSsaTransform tests do a bit much work. I get that you took that from unusedStoreEliminator tests but it in no way reflects the documented prerequisites. Also not in the already existing tests.
Beyond that I think it would be good to add a reproducer regression test with simply S: as optimizer steps.

...dStoreEliminatorNoSsaTransform/function_parameter_reassigned_as_mstore_location_covering.yul

...StoreEliminatorNoSsaTransform/function_parameter_reassigned_as_mstore_location_unrelated.yul

test/libyul/YulOptimizerTestCommon.cpp

clonker

The introduced field should be fixed (also in bugs_by_version.json), beyond that one tiny cosmetic thing that would help improve (my) understanding of a comment in the code, otherwise LGTM!

clonker · 2026-03-25T10:10:02Z

docs/bugs.json

+        "summary": "When the UnusedStoreEliminators optimizer step is run on non-SSA AST form, it may result in incorrect removal of storage or memory writes.",
+        "description": "Solidity allows defining custom user-defined optimizer sequences. In version 0.8.12, the unused store eliminator was introduced to remove redundant writes to storage or memory. The bug was in this optimizer step's implementation. It used SSAValueTracker to identify variables that are never assigned. Based on this set of unassigned variables, a knowledge base is created to track current variable values. This knowledge is necessary for deciding whether memory or storage writes can be safely removed. Unfortunately, SSAValueTracker correctly removed assigned variables from the SSA variable set, but failed to remove variables that depend on those assigned variables. This oversight made it possible to construct inline assembly code that was incorrectly optimized by removing necessary writes. As a result, contracts compiled with different optimization sequences could exhibit different runtime behavior. The bug was introduced in version 0.8.12 alongside the unused store eliminator implementation. It was fixed in version 0.8.35 by implementing additional filtering of the variable set returned by SSAValueTracker and extending this set by functions parameters values.",
+        "link": "https://blog.soliditylang.org/2026/X/Y/Z/",
+        "introduced": "0.8.12",


Suggested change

"introduced": "0.8.12",

"introduced": "0.8.13",

didn't exist in 0.8.12

clonker · 2026-03-25T10:26:26Z

...dStoreEliminatorNoSsaTransform/store_assigned_function_parameter_location_no_elimination.yul

+        // It should be eliminated, because the location is the same, but if we look at the result, second pass of the
+        // optimizer will eliminate properly first `sstore`.


I had to read that comment like five times before I understood it :D Perhaps this is better for dummies like me?

Suggested change

// It should be eliminated, because the location is the same, but if we look at the result, second pass of the

// optimizer will eliminate properly first `sstore`.

// the first sstore could theoretically be eliminated since loc == arg,

// but USE cannot prove this without a known constant value for arg;

// a subsequent optimizer pass (after, e.g., CSE resolves the alias) would eliminate it

purely cosmetic of course, it's also fine as-is.

Co-authored-by: Nikola Matić <nikola.matic@ethereum.org>

…lues to be null.

cameel · 2026-03-27T13:58:44Z

docs/bugs.json

+        "link": "https://blog.soliditylang.org/2026/X/Y/Z/",
+        "introduced": "0.8.13",
+        "fixed": "0.8.35",
+        "severity": "low"


Just a note that one of the blockers for this is having a release date and severity agreed on by the team.

I don't think this one can get into the next release if we're going to have one soon. We need to deal with #16508 first.

rodiazet requested a review from clonker February 10, 2026 13:24

rodiazet force-pushed the fix-mstore-removal branch 2 times, most recently from c987b2b to 38fd9ac Compare February 11, 2026 10:56

cameel added the optimizer label Feb 11, 2026

rodiazet force-pushed the fix-mstore-removal branch 3 times, most recently from 16b2cc9 to 30c820c Compare February 19, 2026 15:58

stackenbotten3000 reviewed Feb 19, 2026

View reviewed changes

rodiazet force-pushed the fix-mstore-removal branch from 30c820c to 2a78055 Compare February 19, 2026 16:23

stackenbotten3000 reviewed Feb 19, 2026

View reviewed changes

rodiazet force-pushed the fix-mstore-removal branch 3 times, most recently from cba7f5b to 31b25f9 Compare February 19, 2026 17:15

rodiazet marked this pull request as ready for review February 19, 2026 17:15

rodiazet requested a review from Copilot February 19, 2026 17:15

Copilot started reviewing on behalf of rodiazet February 19, 2026 17:16 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

libyul/optimiser/SSAValueTracker.h Outdated Show resolved Hide resolved

...StoreEliminatorNoSsaTransform/function_parameter_reassigned_as_mstore_location_unrelated.yul Show resolved Hide resolved

libyul/optimiser/SSAValueTracker.cpp Outdated Show resolved Hide resolved

rodiazet force-pushed the fix-mstore-removal branch 2 times, most recently from 9cf23ac to 7d014ec Compare February 19, 2026 18:26

nikola-matic reviewed Feb 20, 2026

View reviewed changes

rodiazet force-pushed the fix-mstore-removal branch 2 times, most recently from 3ef065c to df081a5 Compare February 20, 2026 12:44

nikola-matic reviewed Feb 20, 2026

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

rodiazet force-pushed the fix-mstore-removal branch from bbdfc8c to b871cd5 Compare February 20, 2026 12:55

cameel reviewed Feb 20, 2026

View reviewed changes

cameel mentioned this pull request Feb 20, 2026

Fix optimising out returndatacopy call. #16436

Closed

rodiazet force-pushed the fix-mstore-removal branch from b871cd5 to 8f69008 Compare February 23, 2026 11:50

nikola-matic reviewed Feb 23, 2026

View reviewed changes

test.sol Outdated Show resolved Hide resolved

rodiazet force-pushed the fix-mstore-removal branch 2 times, most recently from 87e9ec9 to ceee60a Compare February 23, 2026 13:48

rodiazet marked this pull request as ready for review February 23, 2026 15:05

rodiazet force-pushed the fix-mstore-removal branch 2 times, most recently from 58a2f46 to ccc99a9 Compare February 25, 2026 16:30

rodiazet requested a review from nikola-matic February 27, 2026 12:06

clonker reviewed Mar 2, 2026

View reviewed changes

rodiazet force-pushed the fix-mstore-removal branch from 6352590 to 1b21aa9 Compare March 9, 2026 14:31

clonker reviewed Mar 19, 2026

View reviewed changes

rodiazet force-pushed the fix-mstore-removal branch 3 times, most recently from e63d28b to 7db0929 Compare March 24, 2026 11:23

clonker reviewed Mar 25, 2026

View reviewed changes

rodiazet and others added 12 commits March 26, 2026 17:18

Add tests

0f2c6d2

Co-authored-by: Nikola Matić <nikola.matic@ethereum.org>

Improve SSAValueTracker. Fix the bug.

84179d7

Co-authored-by: Nikola Matić <nikola.matic@ethereum.org>

Update changelog.

81c4b0d

Buglist entry

ffa841a

Avoid double lookup. This change allows to Expression pointer in m_va…

bc29eab

…lues to be null.

Some gas gains. To be checked.

0e46bb8

update bug description.

8d46f6b

Add tests covering function parama variables

db09a87

Add memoization to SSAValueTracker::isSSAWithDependencies

87dd6eb

Address review comments.

704ca99

Address review comments. Add cmdLineTest

b97322a

Address PR review comments

0ac2520

rodiazet force-pushed the fix-mstore-removal branch from 7db0929 to 0ac2520 Compare March 26, 2026 16:18

rodiazet requested review from cameel and clonker March 26, 2026 16:19

cameel requested changes Mar 27, 2026

View reviewed changes


		#include <libyul/optimiser/SSAValueTracker.h>

		#include "liblangutil/Exceptions.h"

		// It should be eliminated, because the location is the same, but if we look at the result, second pass of the
		// optimizer will eliminate properly first `sstore`.

-        // It should be eliminated, because the location is the same, but if we look at the result, second pass of the
-        // optimizer will eliminate properly first `sstore`.
+        // the first sstore could theoretically be eliminated since loc == arg,
+        // but USE cannot prove this without a known constant value for arg;
+        // a subsequent optimizer pass (after, e.g., CSE resolves the alias) would eliminate it

Conversation

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

Uh oh!

Uh oh!

stackenbotten3000 commented Feb 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stackenbotten3000 commented Feb 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikola-matic left a comment • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stackenbotten3000 commented Feb 20, 2026

Uh oh!

Uh oh!

cameel Feb 20, 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 Feb 23, 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clonker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clonker left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rodiazet commented Feb 10, 2026 •

edited

Loading

nikola-matic left a comment •

edited

Loading

cameel Feb 20, 2026 •

edited

Loading

rodiazet Feb 23, 2026 •

edited

Loading