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

Fix Via-IR bytecode divergence when compiling multiple files that are already included via imports #14562

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Sep 12, 2023

Fixes #14541.

@r0qs r0qs force-pushed the fix-viaIR-codegen-import-bug branch from f840539 to 507160c Compare September 12, 2023 14:24
@ekpyron
Copy link
Member

ekpyron commented Sep 13, 2023

Ah, ok - you said that bytecode comparison was failing - I thought you only meant in the new "randomized AST IDs" version, but it looks like it's also failing just due to this change?
I guess that means that we do not always add functions to this queue in a deterministic order after all, so we still need to check that.

@cameel
Copy link
Member

cameel commented Sep 13, 2023

It's probably the fact that AscendingFunctionIDCompare was removed from the definition of DispatchSet so it now sorts based on pointer addresses.

@nikola-matic
Copy link
Collaborator

It's probably the fact that AscendingFunctionIDCompare was removed from the definition of DispatchSet so it now sorts based on pointer addresses.

This will just default to std::less, which should have well defined total ordering, right?

@cameel
Copy link
Member

cameel commented Sep 13, 2023

This will just default to std::less, which should have well defined total ordering, right?

Ordering is well defined, but it does not help when addresses are different on each run :)

@nikola-matic
Copy link
Collaborator

This will just default to std::less, which should have well defined total ordering, right?

Ordering is well defined, but it does not help when addresses are different on each run :)

Ah, right as always @cameel :)

@r0qs
Copy link
Member Author

r0qs commented Sep 13, 2023

It's probably the fact that AscendingFunctionIDCompare was removed from the definition of DispatchSet so it now sorts based on pointer addresses.

Yes, I thought that it should not interfere in code generation, but I guess I was wrong. The function IRGenerationContext::initializeInternalDispatch iterates over the set and calls enqueueFunctionForCodeGeneration, so it affects the order as well, as you pointed out. I completely replaced the set by a queue, and this should fix the problem now.

@r0qs r0qs force-pushed the fix-viaIR-codegen-import-bug branch 3 times, most recently from 6e99d76 to 153459f Compare September 13, 2023 15:39
@r0qs r0qs self-assigned this Sep 13, 2023
@r0qs r0qs force-pushed the fix-viaIR-codegen-import-bug branch from 153459f to 6315b07 Compare September 14, 2023 09:29
@r0qs r0qs marked this pull request as ready for review September 14, 2023 09:29
@cameel
Copy link
Member

cameel commented Sep 14, 2023

It would be best if we could add the repro from the issue as a test. How about adding a custom CLI test in the same vein as e.g. ~via_ir_equivalence?

BTW, note that these custom CLI test can be written directly in Python. The current ones are all Bash simply because they used to be a part of cmdlineTests.sh.

@r0qs
Copy link
Member Author

r0qs commented Sep 18, 2023

It would be best if we could add the repro from the issue as a test. How about adding a custom CLI test in the same vein as e.g. ~via_ir_equivalence?

BTW, note that these custom CLI test can be written directly in Python. The current ones are all Bash simply because they used to be a part of cmdlineTests.sh.

Indeed. I did it in another branch to ease the review process. Please let me know if that is what you have in mind #14565.

@r0qs r0qs force-pushed the fix-viaIR-codegen-import-bug branch from 6315b07 to 071bb35 Compare September 18, 2023 08:44
@r0qs r0qs force-pushed the fix-viaIR-codegen-import-bug branch 2 times, most recently from f2e2232 to fc49295 Compare September 25, 2023 13:36
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.

Needs some small tweaks, but overall looks like a good solution.

libsolidity/codegen/ir/IRGenerationContext.cpp Outdated Show resolved Hide resolved
libsolidity/codegen/ir/IRGenerationContext.cpp Outdated Show resolved Hide resolved
Changelog.md Outdated Show resolved Hide resolved
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.

In terms of correctness this looks fine.

I added some small tweaks. If you're fine with them, please squash and we can merge it.

Co-authored-by: Kamil Śliwak <kamil.sliwak@codepoets.it>
@r0qs r0qs force-pushed the fix-viaIR-codegen-import-bug branch from ca75a35 to e3b36f7 Compare September 26, 2023 12:03
@cameel cameel merged commit 2b7ec23 into develop Sep 29, 2023
@cameel cameel deleted the fix-viaIR-codegen-import-bug branch September 29, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Via-IR bytecode differences when explicitly requesting a file that would be pulled by an import
4 participants