fix: linking required LLVM libs only#339
fix: linking required LLVM libs only#339dragon-archer wants to merge 2 commits intoclice-io:mainfrom
Conversation
This patch restricts the GLOB expression to only LLVM & Clang libraries, avoiding linking to a large list of libraries when using system LLVM. Also, this patch explicitly avoids linking to `LLVM-<ver>` and `clang-cpp`, as they will cause lots of duplicate symbol errors
📝 WalkthroughWalkthroughThe CMake LLVM setup was narrowed from a broad lib/* glob to explicit globs for LLVM and Clang libraries and added discovery of compression libraries (z, zstd); target_link_libraries was updated to link the LLVM, Clang, and other required libs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
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: 1
Fix all issues with AI Agents 🤖
In @cmake/llvm.cmake:
- Around line 106-109: The hardcoded entries in OTHER_REQUIRED_LIBS using
"${LLVM_INSTALL_PATH}/lib/libz${CMAKE_STATIC_LIBRARY_SUFFIX}" and
"${LLVM_INSTALL_PATH}/lib/libzstd${CMAKE_STATIC_LIBRARY_SUFFIX}" are brittle;
replace that block with robust discovery: call find_library(Z_LIB z NAMES z zlib
PATHS "${LLVM_INSTALL_PATH}/lib" NO_DEFAULT_PATH OPTIONAL) and
find_library(ZSTD_LIB zstd NAMES zstd libzstd PATHS "${LLVM_INSTALL_PATH}/lib"
NO_DEFAULT_PATH OPTIONAL), then set(OTHER_REQUIRED_LIBS ${Z_LIB} ${ZSTD_LIB})
with fallbacks to system-discovered libraries (call find_library without PATHS
if the NO_DEFAULT_PATH lookups fail) and only keep the explicit
LLVM_INSTALL_PATH-specific paths if the file exists; update references to
CMAKE_STATIC_LIBRARY_SUFFIX only when verifying a discovered path file
existence.
🧹 Nitpick comments (1)
cmake/llvm.cmake (1)
110-110: Add validation to ensure required libraries were found.If the GLOB patterns don't match any libraries (e.g., due to an incorrect installation path or the overly restrictive pattern), the build will configure successfully but fail during linking with potentially cryptic errors. Consider adding checks to verify that essential libraries were discovered.
🔎 Add library discovery validation
) target_link_libraries(llvm-libs INTERFACE ${LLVM_LIBRARIES} ${CLANG_LIBRARIES} ${OTHER_REQUIRED_LIBS}) + + # Validate that we found the required libraries + if(NOT LLVM_LIBRARIES) + message(FATAL_ERROR "No LLVM libraries found at ${LLVM_INSTALL_PATH}/lib/ matching pattern libLLVM*") + endif() + if(NOT CLANG_LIBRARIES) + message(FATAL_ERROR "No Clang libraries found at ${LLVM_INSTALL_PATH}/lib/ matching pattern libclang*") + endif() + + message(STATUS "Found ${CMAKE_MATCH_COUNT} LLVM libraries") + message(STATUS "Found ${CMAKE_MATCH_COUNT} Clang libraries") + target_compile_definitions(llvm-libs INTERFACE CLANG_BUILD_STATIC=1) endif()Note: You may want to also log the count or a sample of found libraries for debugging purposes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmake/llvm.cmake
🔇 Additional comments (1)
cmake/llvm.cmake (1)
104-105: The GLOB pattern is correct and does not need adjustment.LLVM component libraries follow the naming pattern
libLLVM<component>.awhere components contain only letters (e.g.,libLLVMCore.a,libLLVMX86CodeGen.a,libLLVMAArch64CodeGen.a). The patternlibLLVM[a-zA-Z]*appropriately matches these component libraries while correctly excluding the versioned monolithic librarylibLLVM-<major>.soandclang-cpp, which aligns with the PR objectives.
Also, use CMAKE_STATIC_LIBRARY_PREFIX to make the GLOB pattern more robust
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmake/llvm.cmake (1)
106-111: Consider implementing the find_library fallback mentioned in the TODO.The current GLOB approach is an improvement over hardcoded paths, but as noted in the TODO comment and the past review feedback, it still has limitations. If zlib or zstd are not present in
${LLVM_INSTALL_PATH}/lib(which is common when using system LLVM or custom builds), the build will silently proceed without them, potentially causing linker errors later.The past review comment suggests using
find_library()with fallbacks to system libraries, which would be more robust for custom LLVM installations.Based on the past review comment and best practices for CMake library discovery:
🔎 More robust library discovery with find_library
- # TODO: find a better way to find out whether zlib and zstd are needed - # Currently link if present in the LLVM lib directory - file(GLOB OTHER_REQUIRED_LIBS CONFIGURE_DEPENDS - "${LLVM_INSTALL_PATH}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}z${CMAKE_STATIC_LIBRARY_SUFFIX}" - "${LLVM_INSTALL_PATH}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}zstd${CMAKE_STATIC_LIBRARY_SUFFIX}" - ) + # Find compression libraries: first try LLVM installation, then fall back to system + set(OTHER_REQUIRED_LIBS) + + find_library(Z_LIBRARY + NAMES z zlib + PATHS "${LLVM_INSTALL_PATH}/lib" + NO_DEFAULT_PATH + ) + if(NOT Z_LIBRARY) + find_library(Z_LIBRARY NAMES z zlib) + endif() + if(Z_LIBRARY) + list(APPEND OTHER_REQUIRED_LIBS "${Z_LIBRARY}") + endif() + + find_library(ZSTD_LIBRARY + NAMES zstd libzstd + PATHS "${LLVM_INSTALL_PATH}/lib" + NO_DEFAULT_PATH + ) + if(NOT ZSTD_LIBRARY) + find_library(ZSTD_LIBRARY NAMES zstd libzstd) + endif() + if(ZSTD_LIBRARY) + list(APPEND OTHER_REQUIRED_LIBS "${ZSTD_LIBRARY}") + endif()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmake/llvm.cmake
🔇 Additional comments (2)
cmake/llvm.cmake (2)
112-112: LGTM!The linking command correctly incorporates all three library sets (LLVM, Clang, and auxiliary libraries). The separation into distinct variables improves clarity and maintainability.
104-105: The pattern[a-zA-Z]*is correct and sufficient for matching LLVM and Clang static libraries.LLVM static libraries use component-based naming (e.g.,
libLLVMSupport.a,libLLVMAnalysis.a,libclangAST.a) with no digits or special characters immediately after the prefix. Version numbers (like-15) appear only on shared libraries (libLLVM-15.so), not static archives. The pattern correctly excludesLLVM-<ver>andclang-cppas intended.
1 participant
This patch restricts the GLOB expression to only LLVM & Clang libraries, avoiding linking to a large list of libraries when using system LLVM.
Also, this patch explicitly avoids linking to
LLVM-<ver>andclang-cpp, as they will cause lots of duplicate symbol errorsBelow is a small set of errors generated when linking to
clang-cpp:Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.