Add ControlFlowSideEffects parameter to createFunction in EVMDialect
#15535
Conversation
rodiazet
commented
Oct 22, 2024
rodiazet
commented
- This is required by eof contract creation to set control flow side effect nicely .
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
rodiazet
force-pushed
the
extend-create-function-interface
branch
3 times, most recently
from
e2d3f5a
to
6bb5518
Compare
rodiazet
force-pushed
the
extend-create-function-interface
branch
from
6bb5518
to
063b5be
Compare
| SideEffects::Write, // memory | ||
| SideEffects::None // transientStorage | ||
| }, | ||
| ControlFlowSideEffects::fromInstruction(evmasm::Instruction::CODECOPY), |
There was a problem hiding this comment.
I wanted to suggest to use EVMDialect::sideEffectsOfInstruction(evmasm::Instruction::CODECOPY) for SideEffects here too, but apparently the movableApartFromEffects flag here differs from what SemanticInformation::movableApartFromEffects() returns for this instruction.
The discrepancy Looks like a bug to me and confirms that we should be getting these values from SemanticInformation as a single source of truth instead of repeating them. Unless this case is actually intentional? @ekpyron?
There was a problem hiding this comment.
I'm only seeing this now - it shouldn't actually make a difference in this case, but yeah, we can use the SemanticInformation version instead.
There was a problem hiding this comment.
(as in, we probably should, the fact that it doesn't make a difference is more of a happy accident than intent here)
There was a problem hiding this comment.
Good. Here's a PR that fixes it: #15556.
Turns out that the flag was randomly flipped in a refactor: #9283 (comment). Chris actually discovered that and requested a change, and it was marked as resolved, but the fix was apparently never applied before the PR was merged.
Fortunately looks like the value of this flag does not change anything for instructions that write to memory and this is the case here, so nothing was actually broken.
|
I'd merge this one already, but apparently github has not noticed that CI passed after I reran it. Maybe try force pushing it to retrigger the checks (after amending it without changes just to alter the commit hash and make it look like a new one). |
Head branch was pushed to by a user without write access
rodiazet
force-pushed
the
extend-create-function-interface
branch
from
063b5be
to
cbb604e
Compare
