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

Emit acquire fence before loading final field #3699

Merged
merged 1 commit into from Jan 21, 2024

Conversation

armanbilge
Copy link
Member

@armanbilge armanbilge commented Jan 20, 2024

In typelevel/cats-effect#3899 (comment) @durban brought to my attention that the current implementation of final Field Semantics when loading a final field is not correct.

Specifically, we must issue an aquire fence prior to loading the final field. This prevents the load of the field from being re-ordered before the load of the object reference (e.g. due to a compiler optimization that attempts an early, speculative load). Such a re-ordering could cause a pre-initialized value for the field to be observed.

Unfortunately I'm still not confident that we will have a correct implementation of final Field Semantics after this change. However, I do believe that this change brings us closer to that.

On the other end of the spectrum, I am hopeful that we might eventually be able to remove this fence entirely. Ideally, the dependency of the load of the final field on the load of the object reference would enforce the ordering. I don't have sufficient imagination to see how the compiler could generate speculative code that violates this ordering. If we can be confident that neither LLVM nor supported hardware will perform such a re-ordering, then it could be safe to remove this fence.

@WojciechMazur
Copy link
Contributor

Don't worry about any failure in the CI. These are related to the bugs in the GC and I'm trying to fix them

@WojciechMazur WojciechMazur merged commit 6cc47a3 into scala-native:main Jan 21, 2024
55 of 62 checks passed
WojciechMazur added a commit to WojciechMazur/scala-native that referenced this pull request Jan 24, 2024
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.

None yet

2 participants