docs: record the environment an ASan build needs - #12736
Conversation
Configuring with SLANG_ENABLE_ASAN=ON fails partway through a build unless the ASan runtime is on LD_LIBRARY_PATH and leak detection is disabled: the build runs its own instrumented generators (slang-embed, slang-fiddle), which cannot start without the runtime and whose leaks make LeakSanitizer fail the build. Neither failure mentions the sanitizer, so the cause is not obvious. Also note that the build's lib/ directory must be on LD_LIBRARY_PATH when running the binaries, since slang-test dlopen()s its tools and reports the affected tests as ignored -- not failed -- when that lookup fails.
📝 WalkthroughWalkthroughChangesThe build documentation now describes AddressSanitizer and ThreadSanitizer builds, runtime-library paths, LeakSanitizer settings, dynamic tool loading, and the related CI configuration. Sanitizer Documentation
Suggested reviewers: Merge Risk: 🟡 Moderate · up to The new sanitizer guidance can suppress leak reports in later tests, mislead users on unsupported platforms or architectures, and create an unsafe empty library-search-path entry. These documentation errors can cause misleading test results or invalid runtime setup, so the PR is not ready to merge without correction or explicit owner acceptance. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2c7510e4-37b7-482b-a474-4d8053073386
📒 Files selected for processing (1)
docs/building.md
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
| Configuring with `-DSLANG_ENABLE_ASAN=ON` needs two environment settings that are easy to miss, | ||
| because without them the build fails partway through for reasons that do not mention the sanitizer. | ||
|
|
||
| ```bash | ||
| # 1. The build compiles and then *runs* its own generators (slang-embed, slang-fiddle). Under | ||
| # SLANG_ENABLE_ASAN they are instrumented and link the ASan runtime dynamically, so the runtime | ||
| # must be locatable or they fail to start: | ||
| # slang-embed: error while loading shared libraries: libclang_rt.asan-x86_64.so: | ||
| # cannot open shared object file: No such file or directory | ||
| export LD_LIBRARY_PATH="$(clang -print-runtime-dir):$LD_LIBRARY_PATH" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'SLANG_ENABLE_(ASAN|TSAN)|shared-libsan|CMAKE_(C|CXX)_COMPILER_ID' \
cmake/CompilerFlags.cmake .github/workflows/ci-slang-sanitizer.yml docs/building.mdRepository: shader-slang/slang
Length of output: 26757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- docs/building.md ---'
sed -n '611,652p' docs/building.md
printf '%s\n' '--- sanitizer option definitions and compiler branches ---'
rg -n -C 5 \
'SLANG_ENABLE_(ASAN|TSAN)|shared-libsan|fsanitize=(address|thread)|/fsanitize=address|CMAKE_CXX_COMPILER_ID' \
CMakeLists.txt cmake docs/building.md .github/workflows/ci-slang-sanitizer.ymlRepository: shader-slang/slang
Length of output: 24874
Limit the sanitizer instructions to Linux Clang builds.
The CMake implementation uses -shared-libsan only for non-Apple Clang builds. GNU and MSVC use different sanitizer runtime handling, while Apple Clang already uses a dynamic runtime. The TSan example also names the x86_64-specific libclang_rt.tsan-x86_64.so. Scope these instructions to Linux Clang builds, or document the platform- and architecture-specific equivalents.
| # must be locatable or they fail to start: | ||
| # slang-embed: error while loading shared libraries: libclang_rt.asan-x86_64.so: | ||
| # cannot open shared object file: No such file or directory | ||
| export LD_LIBRARY_PATH="$(clang -print-runtime-dir):$LD_LIBRARY_PATH" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
unset LD_LIBRARY_PATH
unsafe="runtime:${LD_LIBRARY_PATH-}"
safe="runtime${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
test "$unsafe" = "runtime:"
test "$safe" = "runtime"Repository: shader-slang/slang
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- docs/building.md ---'
sed -n '600,655p' docs/building.md
printf '%s\n' '--- relevant loader-path references ---'
rg -n -C 3 'LD_LIBRARY_PATH|clang -print-runtime-dir' docs cmakeRepository: shader-slang/slang
Length of output: 6096
🌐 Web query:
Linux ld.so LD_LIBRARY_PATH empty path element current working directory man page
💡 Result:
In the Linux dynamic linker (ld.so), an empty path element within the LD_LIBRARY_PATH environment variable is interpreted as the current working directory [1][2][3]. According to the official ld.so(8) manual page, LD_LIBRARY_PATH is defined as a list of directories separated by colons or semicolons, and a zero-length directory name (e.g., resulting from a leading colon, trailing colon, or consecutive colons like::) explicitly indicates that the dynamic linker should search the current working directory [1][4][3]. This behavior is a legacy feature consistent with the treatment of the PATH environment variable in POSIX systems [5][6]. Because this behavior can cause the linker to load arbitrary libraries from the current directory—potentially leading to security vulnerabilities if an attacker introduces malicious shared objects into that directory—it is generally recommended to avoid empty elements in LD_LIBRARY_PATH [7]. This variable is also ignored entirely when the program is executed in secure-execution mode (such as for set-user-ID or set-group-ID binaries) to mitigate such risks [1][3].
Citations:
- 1: https://man7.org/linux/man-pages/man8/ld.so.8.html
- 2: https://man.archlinux.org/man/core/man-pages/ld.so.8
- 3: https://man.archlinux.org/man/ld.so.8.txt
- 4: https://manpages.ubuntu.com/manpages/jammy/man8/ld-linux.so.8.html
- 5: https://sourceware.org/pipermail/libc-help/2019-May/004832.html
- 6: https://manpages.ubuntu.com/manpages/xenial/man8/ld.so.8.html
- 7: https://jdhao.github.io/2021/07/03/ld_library_path_empty_item/
Avoid empty entries in LD_LIBRARY_PATH.
When LD_LIBRARY_PATH is unset, both commands create a trailing :. Linux treats the empty entry as the current directory, which can load unintended libraries. Use a conditional separator.
| # 2. Those same generators leak, and LeakSanitizer's non-zero exit status fails the build: | ||
| # ERROR: LeakSanitizer: detected memory leaks | ||
| # SUMMARY: AddressSanitizer: 828842 byte(s) leaked in 3661 allocation(s) | ||
| # They are short-lived build tools, so leak detection is not meaningful for them. | ||
| export ASAN_OPTIONS=detect_leaks=0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope ASAN_OPTIONS to the build commands.
The exported variable remains active after cmake --build completes. If the user runs slang-test in the same shell, ASan tests inherit detect_leaks=0, so leak reports are suppressed. Apply the variable only to the configure and build commands, or instruct users to unset it before running tests.
Proposed documentation change
-export ASAN_OPTIONS=detect_leaks=0
-
-cmake --preset default -DSLANG_ENABLE_ASAN=ON
-cmake --build --preset release
+ASAN_OPTIONS=detect_leaks=0 cmake --preset default -DSLANG_ENABLE_ASAN=ON
+ASAN_OPTIONS=detect_leaks=0 cmake --build --preset release![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 🟡 Has issues — 1 gap
This PR adds a "Building with AddressSanitizer" section to docs/building.md documenting the two environment settings (LD_LIBRARY_PATH for the ASan runtime + build lib/, ASAN_OPTIONS=detect_leaks=0) an ASan build/test run needs. The ASan-specific claims all check out against .github/workflows/ci-slang-sanitizer.yml and CMakeLists.txt; one factual error remains: it documents a -DSLANG_ENABLE_TSAN=ON option that the build system does not define.
Changes Overview
ASan/sanitizer build docs (docs/building.md)
- What changed: Adds a new section explaining that under
-DSLANG_ENABLE_ASAN=ONthe build-time generators (slang-embed,slang-fiddle) need the ASan runtime onLD_LIBRARY_PATHandASAN_OPTIONS=detect_leaks=0, and that running the binaries additionally needs the build'slib/onLD_LIBRARY_PATHbecause the sanitizer-intercepteddlopenloses the$ORIGIN/../librunpath. Points atci-slang-sanitizer.ymlfor the CI equivalent.
Findings (1 total)
| Severity | Location | Finding |
|---|---|---|
| 🟡 Gap | docs/building.md:647 |
Documents -DSLANG_ENABLE_TSAN=ON, a build option that does not exist (only SLANG_ENABLE_ASAN is defined) |
Note: the "affected tests are reported as ignored rather than failed" claim was verified accurate — a failed dlopen of the render-test tool makes the -only-startup device probe fail, leaving the render API out of availableRenderApiFlags, so _canIgnore reports dependent tests as TestResult::Ignored (slang-test-main.cpp:1961→1104→5267→5564). The --preset release vs CI's releaseWithDebugInfo and bare clang vs clang-18 differences are acceptable local-dev generality (build/Release/lib is internally consistent with --preset release).
reviewed: 3103397 · diff sha256 d7a5758a1b41
|
|
||
| ```bash | ||
| export LD_LIBRARY_PATH="$PWD/build/Release/lib:$(clang -print-runtime-dir):$LD_LIBRARY_PATH" | ||
| ``` |
There was a problem hiding this comment.
🟡 Gap: -DSLANG_ENABLE_TSAN=ON documents a build option that does not exist
This paragraph tells the reader that the same setup applies to a ThreadSanitizer build enabled via -DSLANG_ENABLE_TSAN=ON, but no such option exists in the build system. The only sanitizer option is SLANG_ENABLE_ASAN (CMakeLists.txt:162, "Enable AddressSanitizer (ASan), and UndefinedBehaviorSanitizer (UBSan) if available as well"), and cmake/CompilerFlags.cmake only implements -fsanitize=address/-fsanitize=undefined under that option — there is no -fsanitize=thread path. A grep of CMakeLists.txt, cmake/, and CMakePresets.json for TSAN/ThreadSanitizer returns zero build-configuration matches, and ci-slang-sanitizer.yml configures ASan only.
Example: A reader following this line runs cmake --preset default -DSLANG_ENABLE_TSAN=ON. CMake emits a "Manually-specified variables were not used by the project" warning and produces an ordinary, non-instrumented build — not a ThreadSanitizer build — so the described libclang_rt.tsan-x86_64.so setup never applies. Since the PR's purpose is to "record the environment an ASan build needs," documenting a toggle the build does not honor is actively misleading.
Suggested fix: Drop the TSAN paragraph (lines 647–648), or gate it as aspirational (e.g. "if a SLANG_ENABLE_TSAN option is added…"), or add the actual CMake option before documenting it.
2 participants
1. Motivation
-DSLANG_ENABLE_ASAN=ONdoes not build out of the box, and neither failure mentions the sanitizer,so the cause is not obvious. Both were hit on a plain Ubuntu 24.04 host with clang-18:
The build compiles its own generators (
slang-embed,slang-fiddle) and then runs them. UnderASan they are instrumented and link the runtime dynamically, so they cannot start unless it is
locatable; and they leak, so LeakSanitizer's non-zero exit fails the build. Both are already handled
in CI (
ci-slang-sanitizer.yml:134and:144), but nothing in the docs says so, so anyonereproducing a sanitizer finding locally rediscovers them.
The first of these is recorded in issue #12707 as a container-specific gotcha. It is not
container-specific — it reproduced on an ordinary host at the identical runtime path. The
LeakSanitizer one is not recorded anywhere.
2. Proposed solution
Add a short "Building with AddressSanitizer" section to
docs/building.mdgiving the two exportsand the error each one prevents, and point at the CI workflow as the authoritative copy.
It also documents a third, run-time requirement that is more insidious than either build failure:
the build's
lib/directory must be onLD_LIBRARY_PATHwhen running the binaries.slang-testdlopens its tools, and when that fails the affected tests are reported ignored, not failed —so the run appears to succeed while silently testing nothing. I hit exactly this and briefly
concluded that ASan was incompatible with Vulkan before finding the real cause.
The section explains why this is specific to sanitizer builds, which is the part that makes it
memorable rather than a rule to copy: normally that
dlopenresolves through the executable's$ORIGIN/../librunpath unaided, but under a sanitizer the call is intercepted and issued from thesanitizer runtime's own object, which has no runpath — so
$ORIGIN/../libis never searched. SettingLD_LIBRARY_PATHto the runtime directory alone is therefore not sufficient, which is the trap,since that is exactly what the first build-time fix tells you to do.
A closing note covers
-DSLANG_ENABLE_TSAN=ON, where the same twoLD_LIBRARY_PATHrequirementsapply with the TSan runtime, and
ASAN_OPTIONS=detect_leaks=0is unnecessary becauseThreadSanitizer has no leak checker. I re-encountered the trap on a TSan build after it had already
been diagnosed on ASan, which is the reason for making the generality explicit.
clang -print-runtime-diris used rather than a hardcoded path so the snippet does not rot acrossclang versions.
3. Change summary
docs/building.mdlib/requirement with its silent-ignore failure mode and the runpath reason it is sanitizer-specific, and a note carrying all of it over toSLANG_ENABLE_TSAN4. Concepts and vocabulary
slang-embed/slang-fiddle— host tools the build compiles and then executes to generatesources. Being instrumented themselves is what makes them sensitive to the ASan runtime.
is unavailable, as ignored. Ignored tests do not fail a run, which is why a missing
LD_LIBRARY_PATHentry removes coverage silently.5. Process report
Documentation only; no behaviour change and nothing to root-cause. The values are not invented —
each is copied from what
ci-slang-sanitizer.ymlalready does, so the docs and CI agree, and thesection says so to keep CI as the single source of truth.
Whether the leaks in
slang-fiddleshould be fixed rather than suppressed is a separate question,deliberately not addressed here:
detect_leaks=0is already CI's chosen behaviour for the buildstep, and changing it is not a documentation matter.
6. Verification
Both exports were required, in sequence, to get a working ASan build on this host — the build failed
at object 511/1188 without the first and at 516/1188 without the second, and completed with both.
The run-time
lib/requirement was confirmed by observingCheck vk,vulkan: Not Supportedand 0tests executing without it, versus vk tests passing with it.