Fix UBSan errors: vptr check failures due to lack of RTTI#9384
Fix UBSan errors: vptr check failures due to lack of RTTI#9384ncelikNV wants to merge 1 commit intoshader-slang:masterfrom
Conversation
Fixes lots of "member call on address [...] which does not point to an object of type '[...]'" UBSan errors, e.g.: ``` .../source/compiler-core/slang-downstream-compiler-set.cpp:88:46: runtime error: member call on address 0x73ce982dc260 which does not point to an object of type 'Slang::IDownstreamCompiler' ``` UBSan's vptr/dynamic type checks require RTTI, as it assumes that a vtable is invalid if the vtable prefix points to a null type info pointer: https://github.com/llvm/llvm-project/blob/ea9addae8336e92222214036bbec821e6b29d8bc/compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp#L215-L217 But as LLVM is built without RTTI by default, slang-llvm was being built without RTTI. Related to shader-slang#9099.
|
As far as I understand, this would require a new release of Slang/slang-llvm to avoid breaking Also, somehow my Btw, that last CMake option, |
| target_compile_options(slang-llvm PRIVATE -wd4244 /Zc:preprocessor) | ||
| endif() | ||
|
|
||
| if(NOT LLVM_ENABLE_RTTI) |
There was a problem hiding this comment.
I think we should keep this if(NOT LLVM_ENABLE_RTTI) block here for the USE_SYSTEM_LLVM option to work. Some distributions may have LLVM without RTTI, and removing this would cause build errors with that option on such distributions.
I think this block shouldn't be running on builds that used build-llvm.sh because of the flag that was added in this PR.
There was a problem hiding this comment.
You're right, I just noticed <LLVM install dir>/lib/cmake/llvm/LLVMConfig.cmake sets LLVM_ENABLE_RTTI to what it was set to when the LLVM CMake build was configured. I thought LLVM_ENABLE_RTTI wouldn't be set here so NOT LLVM_ENABLE_RTTI would always be true.
| -DLLVM_BUILD_TOOLS=0 | ||
| # slang-llvm is built with RTTI enabled to support UndefinedBehaviorSanitizer's vptr checks, so | ||
| # LLVM should be built with RTTI as well | ||
| -DLLVM_ENABLE_RTTI=1 |
There was a problem hiding this comment.
Even if we built LLVM with RTTI enabled in Slang release packages, these won't ship a build of slang-llvm with sanitizer instrumentation enabled.
If we want to instrument slang-llvm with sanitizers as well, we have to build it as part of the sanitizer build instead of downloading the latest Slang release package. If we want to avoid having to build LLVM with RTTI as part of that, I guess we can just use https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#id16 to disable UBSan's vptr checks in slang-llvm (we're unlikely to run into a real type mismatch when casting these Slang COM API types).
2 participants
Fixes lots of
member call on address [...] which does not point to an object of type '[...]'UBSan errors, e.g.:UBSan's vptr/dynamic type checks require RTTI, as it assumes that a
vtable is invalid if the vtable prefix points to a null type info
pointer:
https://github.com/llvm/llvm-project/blob/ea9addae8336e92222214036bbec821e6b29d8bc/compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp#L215-L217
But as LLVM is built without RTTI by default, slang-llvm was being built
without RTTI.
Related to #9099.