search

KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.9k stars 816 forks source link

Fixes undefined behavior due to unspecified bitflag enum backing #3644

Closed MasonRemaley closed 4 days ago

MasonRemaley commented 4 days ago

I build my engine with Zig, which enables LLVM's Undefined Behavior Sanitizer by default in debug builds. UBSAN caught undefined behavior during shader compilation at the following location:

0.  0x000000000426e391 in spv::operator& (a=spv::MemoryAccessMaskNone,
    b=(spv::MemoryAccessVolatileMask | spv::MemoryAccessAlignedMask | spv::MemoryAccessNontemporalMask | spv::MemoryAccessMakePointerVisibleMask | spv::MemoryAccessNonPrivatePointerMask | spv::MemoryAccessAliasScopeINTELMaskMask | spv::MemoryAccessNoAliasINTELMaskMask | unknown: 0xfffcffc0)) at SPIRV/spirv.hpp:2804
1.  0x00000000041e81e0 in (anonymous namespace)::TGlslangToSpvTraverser::accessChainLoad (this=0x7fffffffcbb8, type=...) at SPIRV/GlslangToSpv.cpp:5127
2.  0x00000000041bc059 in (anonymous namespace)::TGlslangToSpvTraverser::visitBinary (this=0x7fffffffcbb8, node=0x5f6a6c8) at SPIRV/GlslangToSpv.cpp:2192
3.  0x0000000003db8660 in glslang::TIntermBinary::traverse (this=0x5f6a6c8, it=0x7fffffffcbb8) at glslang/glslang/MachineIndependent/IntermTraverse.cpp:92
4.  0x0000000003db952e in glslang::TIntermAggregate::traverse (this=0x5f6a7d8, it=0x7fffffffcbb8) at glslang/glslang/MachineIndependent/IntermTraverse.cpp:175
5.  0x00000000042082db in (anonymous namespace)::TGlslangToSpvTraverser::makeGlobalInitializers (this=0x7fffffffcbb8, initializers=...) at SPIRV/GlslangToSpv.cpp:5618
6.  0x00000000041c511f in (anonymous namespace)::TGlslangToSpvTraverser::visitAggregate (this=0x7fffffffcbb8, visit=glslang::EvPreVisit, node=0x5f6a9b8) at SPIRV/GlslangToSpv.cpp:2907
7.  0x0000000003db8fb9 in glslang::TIntermAggregate::traverse (this=0x5f6a9b8, it=0x7fffffffcbb8) at glslang/glslang/MachineIndependent/IntermTraverse.cpp:159
8.  0x00000000041acc04 in glslang::GlslangToSpv (intermediate=..., spirv=..., logger=0x7fffffffdc38, options=0x7fffffffdc2f) at SPIRV/GlslangToSpv.cpp:10398
9.  0x0000000003bfd6f1 in CompileAndLinkShaderUnits (compUnits=...) at StandAlone/StandAlone.cpp:1547
10. 0x0000000003bfe2f8 in CompileAndLinkShaderFiles (Worklist=...) at StandAlone/StandAlone.cpp:1640
11. 0x0000000003bfec2c in singleMain () at StandAlone/StandAlone.cpp:1713
12. 0x0000000003bff4dd in main (argc=5, argv=0x7fffffffe3d8) at StandAlone/StandAlone.cpp:1767

The explanation is that enums when used as bitflags in C++ must declare a backing type. If they don't, you can end up with "out of range" values (for example when bitwise negated as is the case in this trace) leading to undefined behavior when converting to/from integral values.

It's a bit hard to follow, but see the discussion here on C++ enums and undefined behavior.

Thankfully, the fix is easy. This PR simply sets the relevant enum backing types to unsigned int. I chose unsigned int since these enums are already assumed to be unsigned ints in other parts of the code anyway.

CLAassistant commented 4 days ago

CLA assistant check
All committers have signed the CLA.

MasonRemaley commented 3 days ago

This brings the copy of spirv.hpp that glslang uses in line with what is done in the spirv.hpp11 file in the spirv-headers repo, [...]

Out of curiosity, what does the .hpp11 extension mean? I saw it when setting up my build, but am not familiar with the convention.