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

chore: disable non-standard volatile behavior on MSVC #1534

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

jadebenn
Copy link
Collaborator

@jadebenn jadebenn commented Apr 5, 2024

Discovered this at work the other day. By default, MSVC treats all volatile variables as if they were atomics on x86 and x64 architectures, and requires this use of an obscure compiler flag to disable this.

https://learn.microsoft.com/en-us/cpp/build/reference/volatile-volatile-keyword-interpretation?view=msvc-170

This is, needless to say, very much not portable and it makes me worried it could potentially cause odd behavior between the platforms. While there's no evidence it has at the moment, I'd rather nip this in the bud. This PR adds said obscure compiler flag to CMake and thus brings MSVC inline with gcc and clang's treatment of volatile in accordance to the C++ standard.

@jadebenn
Copy link
Collaborator Author

jadebenn commented Apr 5, 2024

I still can't quite get the windows build working on my machine, but there's an MSVC compiler warning that should let us know if this change might cause any issues in our windows-specific dependencies. Otherwise, this should not affect anything shared across platforms and should in fact remove a potential bug source from play.

@aronwk-aaron
Copy link
Member

So to make sure i understand. this would make primitives, like size_t, be consistent across compiles?

@jadebenn
Copy link
Collaborator Author

jadebenn commented Apr 5, 2024

So to make sure i understand. this would make primitives, like size_t, be consistent across compiles?

No, this just has to do with volatile types

@aronwk-aaron aronwk-aaron merged commit 18c27b1 into main Apr 5, 2024
4 checks passed
@aronwk-aaron aronwk-aaron deleted the msvc-volatiles branch April 5, 2024 17:57
@Xiphoseer
Copy link
Contributor

I disagree this is a portability concern and see no downside to volatile access being atomic?

@EmosewaMC
Copy link
Collaborator

I disagree this is a portability concern and see no downside to volatile access being atomic?

i dont either but dont have the energy to refute something like this

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

4 participants