DarkflameUniverse / DarkflameServer

The main repository for the Darkflame Universe Server Emulator project.
https://www.darkflameuniverse.org/
GNU Affero General Public License v3.0
639 stars 172 forks source link

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

Closed jadebenn closed 5 months ago

jadebenn commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

jadebenn commented 5 months ago

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

Xiphoseer commented 5 months ago

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

EmosewaMC commented 5 months ago

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