Skip to content

Fix wrong native locations in optimized Yul artifacts#15309

Merged
ekpyron merged 3 commits intodevelopfrom
fix-wrong-native-locations-in-optimized-yul-artifacts
Aug 12, 2024
Merged

Fix wrong native locations in optimized Yul artifacts#15309
ekpyron merged 3 commits intodevelopfrom
fix-wrong-native-locations-in-optimized-yul-artifacts

Conversation

@cameel
Copy link
Collaborator

@cameel cameel commented Jul 31, 2024

Extends #15228.

The original PR handled only the Yul AST produced from Solidity sources. This one addresses the issue more comprehensively, by moving reparsing of optimized IR to YulStack so that it has effect on pure Yul compilation as well. In Yul mode the affected artifacts are not limited to the AST - source locations and debug comments in EVM assembly output are also affected.

Also fixes a small bug in YulStack that made it report warnings twice (i.e. ones reported in unoptimized code and then the same warnings from optimized code).

@cameel cameel requested review from clonker and ekpyron July 31, 2024 18:10
@cameel cameel self-assigned this Jul 31, 2024
@cameel cameel force-pushed the fix-wrong-native-locations-in-optimized-yul-artifacts branch from b487140 to 0d75360 Compare July 31, 2024 18:18

m_stackState = Parsed;
optimize(*m_parserResult, true);
yulAssert(analyzeParsed(), "Invalid source code after optimization.");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what was causing double warnings. analyzeParsed() is not side-effect free and actually adds anything that is reported to the current error list. The assumption was probably that a this point nothing will be reported because the code compiles. Warnings and infos are still possible though. They will be reported again unless the code that triggers them happens to be optimized out.

public:
static void compile(
Object& _object,
Object const& _object,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why this wasn't const already. It should have been because EVMObjectCompiler does not modify the object. I'm changing it here because I wanted to be sure that optimize() is the only place that actually modifies the object and requires reparsing.

Copy link
Member

Choose a reason for hiding this comment

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

Probably a remnant of findFunctionCalls not having a const version. Still, even a const ref to an object allows modification on the ast, as it's sitting in a smart pointer which doesnt propagate constness. I think this should be changed eventually

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. But at least your immutable AST PR does address it, at least for the AST which is the most relevant part of the object. Perhaps we should just make Object a proper class and hide other members behind const accessors too.

Comment on lines +258 to +259
YulStack cleanStack(m_evmVersion, m_eofVersion, m_language, m_optimiserSettings, m_debugInfoSelection);
bool reanalysisSuccessful = cleanStack.parseAndAnalyze(m_charStream->name(), source);
Copy link
Collaborator Author

@cameel cameel Jul 31, 2024

Choose a reason for hiding this comment

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

Using YulStack in YulStack feels a bit like an overkill, but it seemed cleaner to me than refactoring the parsing methods to make them usable without overwriting what's in the current stack.

@cameel cameel mentioned this pull request Jul 31, 2024
Copy link
Member

@clonker clonker left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me. I am not sure if I am too happy with YulStack as a whole, it does play with a lot of side effects (which of course is somewhat in the nature of combining parsing/analyzing/optimizing/assembling). Maybe it would help a bit to, e.g., have both unoptimized and optimized objects in the state.

public:
static void compile(
Object& _object,
Object const& _object,
Copy link
Member

Choose a reason for hiding this comment

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

Probably a remnant of findFunctionCalls not having a const version. Still, even a const ref to an object allows modification on the ast, as it's sitting in a smart pointer which doesnt propagate constness. I think this should be changed eventually

/// rather than the initial source.
/// @warning Does not update the error list or the original source (@a m_charStream) to make
/// it possible to report errors to the user even after the optimization has been performed.
void reparse();
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like something that could potentially also be solved by keeping the original parsing result / object around and the optimize method create a new, optimized version of it (perhaps in the spirit of the immutable AST PR). Then, I imagine, there also wouldn't be this mix of properties where some refer to data before optimization, some to data after optimization and you could potentially get rid of the YulStack in YulStack implementation of reparse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, that would be a much better design, more similar to CompilerStack. YulStack is just wildly inconsistent. Parsing is stateful but creates new things (stores parsing result), then optimization is stateful but modifies things in place, and then assembling is stateless and not in-place (just returns the result to the caller). It should be more like a proper pipeline with each stage filling its artifact based on the result of previous stages.

Still, I'm not to keen to get into that refactor right now. I'd get rid of YulStack from reparse() but I would still basically have to do the refactor I avoided, i.e. create stateless helpers for parsing and analysis so that I can run them on different objects. I wanted this to be a quick fix so that I get the cache to PR to a mergeable state before I'm off tomorrow.

So, if there are no bugs or bigger issues here, I'd rather merge this as is to unblock the caching PR. We can always do that refactor on top of them, later.

@cameel cameel force-pushed the fix-wrong-native-locations-in-optimized-yul-artifacts branch from 0d75360 to babe3d9 Compare August 1, 2024 21:54
clonker
clonker previously approved these changes Aug 2, 2024
ekpyron
ekpyron previously approved these changes Aug 7, 2024
cameel added 3 commits August 7, 2024 15:48
@clonker clonker dismissed stale reviews from ekpyron and themself via fe4c52e August 7, 2024 13:56
@clonker clonker force-pushed the fix-wrong-native-locations-in-optimized-yul-artifacts branch from babe3d9 to fe4c52e Compare August 7, 2024 13:56
@ekpyron ekpyron merged commit 6d3340a into develop Aug 12, 2024
@ekpyron ekpyron deleted the fix-wrong-native-locations-in-optimized-yul-artifacts branch August 12, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants