Skip to content
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.

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

eof: Fix immutables #15628

Merged
merged 2 commits into from
Dec 12, 2024
Merged

eof: Fix immutables #15628

merged 2 commits into from
Dec 12, 2024

Conversation

rodiazet
Copy link
Contributor

@rodiazet rodiazet commented Dec 9, 2024

Fix immutables bug. EOF implementation needs to have m_immutbalesVariables and m_libraryAddressOffset members of IRGenerationContext properly initialized because their values contains offsets to immutables variables values in EOF data section and they are needed when generating Deployed container.

Enable one semantic test which failed before the fix

Depends on #15626. Merged.
Depends on #15635. Merged.
Closes #15606.

This comment was marked as resolved.

@rodiazet rodiazet force-pushed the eof-fix-immutables branch 2 times, most recently from 582846e to 24b848d Compare December 9, 2024 16:43
@cameel cameel added has dependencies The PR depends on other PRs that must be merged first EOF labels Dec 9, 2024
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

The fix itself seems ok, but I'd change a few minor things.

libsolidity/codegen/ir/IRGenerator.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ir/IRGenerationContext.cpp Outdated Show resolved Hide resolved
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Dec 9, 2024
@rodiazet rodiazet force-pushed the eof-fix-immutables branch 8 times, most recently from 745d1ed to 819a3ca Compare December 10, 2024 11:22
@rodiazet rodiazet force-pushed the eof-fix-immutables branch 2 times, most recently from c4098ca to 52cc020 Compare December 10, 2024 13:51
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Dec 11, 2024
cameel
cameel previously approved these changes Dec 11, 2024
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Fine to merge after #15635.

@cameel
Copy link
Member

cameel commented Dec 11, 2024

Actually, can you also pull in e14830b? That's the only bit from #15606 that's still missing, so if you do that, we can also close it (add Closes #15606 to the description).

@rodiazet
Copy link
Contributor Author

Actually, can you also pull in e14830b? That's the only bit from #15606 that's still missing, so if you do that, we can also close it (add Closes #15606 to the description).

I can pull. I did not do it because it's unrelated change. I wanna also to enable EOF testing by default so this changes won't be needed. Right?

@cameel
Copy link
Member

cameel commented Dec 11, 2024

True in the end it's unrelated. It's just so minor that I thought just a separate commit in some EOF PR was enough of a separation and I was more worried about just forgetting it :)

Personally, I'd rather get it in faster, but you're right, enabling EOF by default will ultimately solve the problem as well.

@rodiazet
Copy link
Contributor Author

True in the end it's unrelated. It's just so minor that I thought just a separate commit in some EOF PR was enough of a separation and I was more worried about just forgetting it :)

Personally, I'd rather get it in faster, but you're right, enabling EOF by default will ultimately solve the problem as well.

OK. I will add it here. Just not to forget about it later.

@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Dec 11, 2024
cameel
cameel previously approved these changes Dec 11, 2024
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Please rebase and we can merge.

@rodiazet rodiazet dismissed cameel’s stale review December 11, 2024 18:53

The merge-base changed after approval.

@cameel cameel merged commit 9ad6660 into ethereum:develop Dec 12, 2024
73 checks passed
@rodiazet rodiazet deleted the eof-fix-immutables branch December 12, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants