Skip to content

Yul: NoOutputAssembly assigns functions instead of appending #16127

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

Merged
merged 1 commit into from
Jul 21, 2025
Merged

Yul: NoOutputAssembly assigns functions instead of appending #16127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions libyul/backends/evm/NoOutputAssembly.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,13 +212,13 @@ NoOutputEVMDialect::NoOutputEVMDialect(EVMDialect const& _copyFrom):
// them in one go, later reference pointers to this static vector
static std::vector<BuiltinFunctionForEVM> noOutputBuiltins = defineNoOutputBuiltins();

m_functions.reserve(m_functions.size());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, so this should in theory never do anything, as reserve will only perform a reallocation if and only if capacity() is smaller than the new requested capacity. size() should thus technically always be smaller or equal to capacity(), as auto reallocation (i.e. capacity increase) is only performed when you want to insert an element that would exceed the container's capacity.

What I'm trying to say is that ASAN was likely wrong here, but this then begs another question - the old implementation appends builtin handles to m_functions, whereas the new one overwrites the current ones (which is good and correct) - but then, how come this wasn't caught in any tests? Do we even have tests for no output builtins?

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue was with emplace_back down below. This is indeed a no-op so I removed it as well. We are looping over m_functions and were simultaneously appending to it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that part I understand - we had bugs like these before, which are usually quite insidious and can't be reproduced via tests quite well; my question was more so aimed at the semantic difference between the two (assume iterator invalidation is not an issue) - old implementation appends, where as the new one overwrites - this should in theory mean that in the old case m_functions will always have more elements than in the new (overwritten) version?

In any case, it's used in the compatibility checker, so no big deal, but still weird that it wasn't caught by some test. Although from what I can see, we have no such tests, so this makes perfect sense :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah the m_functions is populated in the superclass and has to be same in size as the one in the NoOutputDialect that inherits from it. Appending was just wrong but not wrong enough apparently to warrant an outright crash. Semantic difference is that the the compilability checker and stack compressors would have taken the actual builtins, not the ones that are nulled out / stubbed.

yulAssert(m_functions.size() == noOutputBuiltins.size(), "Function count mismatch.");
for (auto const& [index, builtinFunction]: m_functions | ranges::views::enumerate)
{
if (builtinFunction)
m_functions.emplace_back(&noOutputBuiltins[index]);
m_functions[index] = &noOutputBuiltins[index];
else
m_functions.emplace_back(nullptr);
m_functions[index] = nullptr;
}
}

Expand Down