-
Notifications
You must be signed in to change notification settings - Fork 379
Fix SLANG_ENABLE_ASAN CMake option
#9108
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
base: master
Are you sure you want to change the base?
Conversation
…producible example for)
Clang was taking around 10 minutes to compile `slang-embedded-core-module-source.cpp` with optimizations. Disabling optimizations for these functions reduced the compilation time to about 12 seconds on my machine (Intel Core Ultra 7 165H), and had no noticeable impact on run-time performance. Run-time performance with optimizations: ``` $ hyperfine --shell=none './build/generators/Release/bin/slang-bootstrap -archive-type riff-lz4 -save-core-module-bin-source slang-core-module-generated.h -save-glsl-module-bin-source slang-glsl-module-generated.h' Benchmark 1: ./build/generators/Release/bin/slang-bootstrap -archive-type riff-lz4 -save-core-module-bin-source slang-core-module-generated.h -save-glsl-module-bin-source slang-glsl-module-generated.h Time (mean ± σ): 2.545 s ± 0.035 s [User: 2.333 s, System: 0.210 s] Range (min … max): 2.496 s … 2.620 s 10 runs ``` Run-time performance without optimizations: ``` $ hyperfine --shell=none './build/generators/Release/bin/slang-bootstrap -archive-type riff-lz4 -save-core-module-bin-source slang-core-module-generated.h -save-glsl-module-bin-source slang-glsl-module-generated.h' Benchmark 1: ./build/generators/Release/bin/slang-bootstrap -archive-type riff-lz4 -save-core-module-bin-source slang-core-module-generated.h -save-glsl-module-bin-source slang-glsl-module-generated.h Time (mean ± σ): 2.564 s ± 0.039 s [User: 2.350 s, System: 0.213 s] Range (min … max): 2.512 s … 2.614 s 10 runs ``` Disabling optimizations also makes `slang-embedded-core-module-source.cpp.o` slightly smaller: - 7.84 MiB with optimizations, - 7.37 MiB without optimizations. Fixes shader-slang#9054.
… result still unsigned"
|
This PR modifies
This version number helps maintain compatibility when loading serialized IR modules. |
| target_compile_options(${target} PRIVATE /fsanitize=address) | ||
| target_link_options(${target} PRIVATE /INCREMENTAL:NO) | ||
| else() | ||
| message(FATAL_ERROR "SLANG_ENABLE_ASAN: unsupported C++ compiler") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be better to raise a FATAL_ERROR (which stops CMake generation) as users might not notice a warning. Since SLANG_ENABLE_ASAN defaults to OFF I think it's safe to assume that it being ON implies the user really doesn't want Slang being built without sanitizers.
| if (PlatformUtil::isFamily(PlatformFamily::Unix, platformKind)) | ||
| { | ||
| // Position independent | ||
| cmdLine.addArg("-fPIC"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial discussion: https://github.com/shader-slang/slang/pull/8980/files#r2528348258.
GCC used to emit the following warning when using -fPIC on Windows until GCC 6.1 (released in 2016):
-fPIC ignored for target (all code is position independent)
See gcc-mirror/gcc@0a1d992. I assume that's why this was restricted to PlatformFamily::Unix. The commit that added this restriction is from 2019 (~3 years after GCC 6.1's release): ea20066.
|
|
||
| diagnostics->reset(); | ||
| diagnostics->setRaw(SliceUtil::asCharSlice(exeRes.standardError)); | ||
| diagnostics->appendRaw(SliceUtil::asCharSlice(exeRes.standardError)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since CommandLineDownstreamCompiler::compile now splits compilation and linking into two commands, we need to be able to combine multiple commands' output into the same diagnostics object. Note that parseOutput is only ever called on a newly created diagnostics object (even without this PR's changes), so there was no need to call diagnostics->reset() here.
| char* getData() const { return m_buffer ? m_buffer->getData() : (char*)""; } | ||
| char* getData() const | ||
| { | ||
| static char empty[] = ""; | ||
| return m_buffer ? m_buffer->getData() : empty; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial discussion: https://github.com/shader-slang/slang/pull/8980/files#r2518762552.
| bool operator==(const char* strbuffer) const { return (strcmp(begin(), strbuffer) == 0); } | ||
| bool operator==(const char* strbuffer) const | ||
| { | ||
| const char* volatile b = begin(); | ||
| return (strcmp(b, strbuffer) == 0); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial discussion: https://github.com/shader-slang/slang/pull/8980/files#r2518778200.
| Fossil::SerialWriter writer(blobBuilder); | ||
| ASTSerialWriteContext context(moduleDecl, sourceLocWriter); | ||
| Fossil::SerialWriter writer(blobBuilder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes ASan error: stack-use-after-scope.
| ComPtr<ISlangWriter> writer(new FileWriter(stderr, WriterFlag::AutoFlush)); | ||
| ComPtr<ISlangWriter> writer( | ||
| new FileWriter(stderr, WriterFlag::IsUnowned | WriterFlag::AutoFlush)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stderr was being closed when exiting main, which meant sanitizers would fail to print diagnostics.
| const SlangPassThrough cppCompilers[] = { | ||
| SLANG_PASS_THROUGH_VISUAL_STUDIO, | ||
| SLANG_PASS_THROUGH_GCC, | ||
| #if SLANG_CLANG | ||
| SLANG_PASS_THROUGH_CLANG, | ||
| #else | ||
| SLANG_PASS_THROUGH_GCC, | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here was that I was building Slang with Clang but a test was being built with GCC and the sanitizer runtime of one is incompatible with the other's. Maybe this code should just be edited to only test the compiler that is being used to build Slang instead of MSVC + either Clang or GCC, this was more of a quick fix.
| return UnownedStringSlice(); | ||
| return String(); | ||
| } | ||
|
|
||
| return UnownedStringSlice(fileStat.m_filename).trim('/'); | ||
| return String(UnownedStringSlice(fileStat.m_filename).trim('/')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes ASan error: stack-use-after-return.
Fixes sanitizers silently not being enabled due to broken checks for compiler support of sanitizer flags. Fixes non-leak errors reported by ASan when running `slang-test -api cpu` on Linux. Fixes shader-slang#9097. Fixes shader-slang#9098.
1cbaf81 to
d33b400
Compare
| SLANG_CHECK_ABORT(code != nullptr); | ||
|
|
||
| SLANG_CHECK(code->getBufferSize() != 0); | ||
| SLANG_CHECK_ABORT(code->getBufferSize() != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes crash/segfault due to null pointer dereference if libslang-glslang-<version>.so couldn't be found (build/<config>/lib not in LD_LIBRARY_PATH): code would be nullptr.
| SLANG_CHECK(compileResult->getMetadata(metadata.writeRef()) == SLANG_OK); | ||
| SLANG_CHECK_ABORT(result == SLANG_OK); | ||
| SLANG_CHECK_ABORT(compileResult != nullptr); | ||
| SLANG_CHECK_ABORT(compileResult->getItemCount() == 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes crash/segfault due to null pointer dereference if libslang-glslang-<version>.so couldn't be found (build/<config>/lib not in LD_LIBRARY_PATH): result would be != SLANG_OK and compileResult would be nullptr.
Fixes sanitizers silently not being enabled due to broken checks for
compiler support of sanitizer flags.
Fixes non-leak errors reported by ASan when running
slang-test -api cpuon Linux.Fixes #9097.
Fixes #9098.