[DeviceSanitizer] Fix circular library dependency causing undefined symbol - #22901
Conversation
…ymbol Commit 0941896 ("[DeviceSanitizer] Make sanitizer metadata globals per-module unique (intel#22567)") made SanitizerPostOptimizer.cpp in LLVMSYCLLowerIR call sycl::isModuleUsingMsan(), which is defined in LLVMSYCLPostLink. LLVMSYCLPostLink already links against LLVMSYCLLowerIR, so this created a circular library dependency. It doesn't show up when linking against full static LLVM libs, but breaks with `ld.lld: error: undefined symbol:llvm::sycl::isModuleUsingMsan(llvm::Module const&)` when building LLVMSYCLLowerIR as a standalone shared library (-slibs build). Fix by moving isModuleUsingAsan/Msan/Tsan down into SYCLLowerIR/SYCLUtils.{h,cpp}, which has no dependency on SYCLPostLink. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
|
Please review, thanks. This blocks downstream pulldown. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a circular library dependency between LLVMSYCLLowerIR and LLVMSYCLPostLink that could surface as an undefined symbol when building LLVMSYCLLowerIR as a standalone shared library (e.g., -slibs builds). It does so by relocating the sanitizer-usage detection helpers (isModuleUsingAsan/Msan/Tsan) into SYCLLowerIR utilities, removing SYCLLowerIR’s need to reference a symbol defined in SYCLPostLink.
Changes:
- Moved
isModuleUsingAsan/Msan/Tsan(const Module&)implementations fromSYCLPostLink(ComputeModuleRuntimeInfo.cpp) intoSYCLLowerIR(SYCLUtils.{h,cpp}). - Updated
SYCLLowerIRandsycl-jitcall sites to includeSYCLUtils.hinstead of relying onComputeModuleRuntimeInfo.hfor these helpers. - Removed the declarations of these helpers from the
SYCLPostLinkpublic header (ComputeModuleRuntimeInfo.h).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| sycl-jit/jit-compiler/lib/rtc/DeviceCompilation.cpp | Adds SYCLUtils.h include to access the relocated isModuleUsing* helpers. |
| llvm/lib/SYCLPostLink/ComputeModuleRuntimeInfo.cpp | Removes local isModuleUsing* definitions now provided by SYCLLowerIR. |
| llvm/lib/SYCLLowerIR/SYCLUtils.cpp | Adds the new shared implementations of isModuleUsingAsan/Msan/Tsan. |
| llvm/lib/SYCLLowerIR/SanitizerPostOptimizer.cpp | Switches include dependency from SYCLPostLink header to SYCLUtils.h. |
| llvm/include/llvm/SYCLPostLink/ComputeModuleRuntimeInfo.h | Removes isModuleUsing* declarations from the SYCLPostLink API surface. |
| llvm/include/llvm/SYCLLowerIR/SYCLUtils.h | Adds declarations for isModuleUsingAsan/Msan/Tsan (with Module forward declaration). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| bool isModuleUsingAsan(const Module &M); | ||
| bool isModuleUsingMsan(const Module &M); | ||
| bool isModuleUsingTsan(const Module &M); | ||
|
|
There was a problem hiding this comment.
nit:
These three predicates are sanitizer-specific, but SYCLLowerIR/SYCLUtils.h is the generic SYCL grab-bag, and everything else in it lives under sycl::utils. SYCLLowerIR/SanitizerPostOptimizer.h is already the sanitizer-owned header in this component and declaring them there breaks the cycle. Could you please consider it?
A new SYCLLowerIR/SanitizerUtils.h might be an even cleaner solution. Additional argument for SYCLLowerIR/SanitizerUtils.h:
__AsanKernelMetadata / __MsanKernelMetadata / __TsanKernelMetadata now appear as raw literals in SYCLUtils.cpp, SanitizerPostOptimizer.cpp, SanitizerPostSplitProcessing.cpp, clang/lib/Driver/OffloadBundler.cpp, plus the producers in {Address,Memory,Thread}Sanitizer.cpp. New SanitizerUtils.h could be used to also expose constexpr StringRef ASAN_KERNEL_METADATA_PREFIX etc. to have single definition of prefixes in addition to keeping the predicates.
There was a problem hiding this comment.
nit:
These three predicates are sanitizer-specific, but
SYCLLowerIR/SYCLUtils.his the generic SYCL grab-bag, and everything else in it lives undersycl::utils.SYCLLowerIR/SanitizerPostOptimizer.his already the sanitizer-owned header in this component and declaring them there breaks the cycle. Could you please consider it?A new
SYCLLowerIR/SanitizerUtils.hmight be an even cleaner solution. Additional argument forSYCLLowerIR/SanitizerUtils.h:__AsanKernelMetadata/__MsanKernelMetadata/__TsanKernelMetadatanow appear as raw literals inSYCLUtils.cpp,SanitizerPostOptimizer.cpp,SanitizerPostSplitProcessing.cpp,clang/lib/Driver/OffloadBundler.cpp, plus the producers in {Address,Memory,Thread}Sanitizer.cpp. NewSanitizerUtils.hcould be used to also exposeconstexpr StringRef ASAN_KERNEL_METADATA_PREFIXetc. to have single definition of prefixes in addition to keeping the predicates.
thanks @YuriPlyakhin for the review. These are good points. Follow-up the ideas at #22915
Commit 0941896 ("[DeviceSanitizer] Make sanitizer metadata globals per-module unique (#22567)") made SanitizerPostOptimizer.cpp in LLVMSYCLLowerIR call sycl::isModuleUsingMsan(), which is defined in LLVMSYCLPostLink. LLVMSYCLPostLink already links against LLVMSYCLLowerIR, so this created a circular library dependency. It doesn't show up when linking against full static LLVM libs, but breaks with
ld.lld: error: undefined symbol:llvm::sycl::isModuleUsingMsan(llvm::Module const&)when building LLVMSYCLLowerIR as a standalone shared library (-slibs build).Fix by moving isModuleUsingAsan/Msan/Tsan down into SYCLLowerIR/SYCLUtils.{h,cpp}, which has no dependency on SYCLPostLink.