Handle clang/gcc defining "unix" during compilation #121097
Conversation
The Emscripten tool chain defines 'unix' during compilation. This meant that during the data contract generation the "unix" string was converted to "1" and interpreted as "windows". See `./src/mono/browser/emsdk/emscripten/emcc -dM -E - < /dev/null | grep unix`. The fix here is to create a specific OS for the browser and have that run first. Place some defensive checks in place for the other OS names that aren't all that unique either.
AaronRobinsonMSFT
requested review from
Copilot,
jkoritzinsky,
kg,
max-charlamb and
rcj1
AaronRobinsonMSFT
added
arch-wasm
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the Emscripten toolchain's definition of unix during compilation caused incorrect operating system detection in the data contract generation, making "unix" resolve to "1" which was then interpreted as "windows". The fix introduces a specific Browser operating system value and reorders the OS detection to check for browser first, while adding defensive compile-time checks for all OS name defines.
Key Changes:
- Added
Browseras a newRuntimeInfoOperatingSystemenum value - Changed OS comparisons from
== Unixto!= Windowsto handle the new Browser OS - Added compile-time checks to detect and fail early if OS names are defined as macros
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| datadescriptor.inc | Added Browser OS target detection (checked first), defensive #error directives for OS name defines |
| IRuntimeInfo.cs | Added Browser enum value to RuntimeInfoOperatingSystem |
| SOSDacImpl.cs | Changed condition from == Unix to != Windows to handle Browser OS |
| X86Unwinder.cs | Changed Unix ABI detection from == Unix to != Windows |
| AMD64Unwinder.cs | Changed Unix detection from == Unix to != Windows |
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs
Show resolved
Hide resolved
This hopefully avoids the common case of defining unix on many compilers.
AaronRobinsonMSFT
changed the title
|
Apparently specifying a standard like c99 will suppress definition of the unix keyword, but it's probably for the best to handle it explicitly like you're doing here. |
....Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64/AMD64Unwinder.cs
Outdated
Show resolved
Hide resolved
|
LGTM but will leave the green check to someone who knows cdac better |
....Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64/AMD64Unwinder.cs
Outdated
Show resolved
Hide resolved
....Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64/AMD64Unwinder.cs
Show resolved
Hide resolved
Oh didn't realize that. I agree I don't think it should change the defensive checks made in this PR, but it is good to know - Thanks! |
4 participants
The clang and gcc tool chains defines 'unix' during compilation - clang doesn't on macOS. This means that during the data contract generation the "unix" string was converted to "1" and interpreted as "windows". See
./src/mono/browser/emsdk/emscripten/emcc -dM -E - < /dev/null | grep unixorclang -dM -E - < /dev/null | grep unixon a linux distro.The first step is to create a specific OS for the browser and then make all string definitions case sensitive so the "unix" define doesn't convert. We also place some defensive checks in place for the OS names that aren't all that unique either.
Alternative 1 Set the enum values as opposed to the "string" names - "1" as opposed to "Windows". This would be more difficult to maintain, but unlikely to be conflicting defines.
Alternative 2 Define the
RuntimeInfoOperatingSystemvalues to not be low values so they would rarely resolve to "valid" enumeration values. This would create easy to find failures at run-time.