link correct lib for asan - #286
Conversation
WalkthroughAdds conditional ASAN support to the eloq data store build: introduces compile definitions for ASAN, selects an ASAN-specific Boost.Context library when requested, and abstracts Boost.Context linkage behind a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
store_handler/eloq_data_store_service/build_eloq_store.cmake (1)
19-34: Add error handling for missing ASAN Boost library and make path configurable.Two concerns with this block:
Hardcoded path: The path
$ENV{HOME}/boost_ucontext_asanassumes a specific directory structure in the user's home. Consider making this a configurable cache variable with a default.Missing error check:
find_librarysetsBoost_CONTEXT_LIBRARYtoNOTFOUNDif the library isn't found, which will cause a confusing link error later rather than a clear configuration error.if(WITH_ASAN) message("build eloqstore with ASAN: ${WITH_ASAN}") - set(BOOST_CONTEXT_ASAN_PATH "$ENV{HOME}/boost_ucontext_asan") + set(BOOST_CONTEXT_ASAN_PATH "$ENV{HOME}/boost_ucontext_asan" CACHE PATH "Path to ASAN-compatible Boost.Context installation") add_compile_definitions(BOOST_USE_ASAN) add_compile_definitions(BOOST_USE_UCONTEXT) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -fno-omit-frame-pointer") find_library(Boost_CONTEXT_LIBRARY NAMES boost_context PATHS ${BOOST_CONTEXT_ASAN_PATH}/lib NO_DEFAULT_PATH) + if(NOT Boost_CONTEXT_LIBRARY) + message(FATAL_ERROR "Failed to find ASAN-compatible boost_context in ${BOOST_CONTEXT_ASAN_PATH}/lib") + endif() set(BOOST_CONTEXT_TARGET ${Boost_CONTEXT_LIBRARY}) else ()store_handler/eloq_data_store_service/CMakeLists.txt (1)
151-152: Duplicate compile definitions.
BOOST_USE_ASANandBOOST_USE_UCONTEXTare also added inbuild_eloq_store.cmake(lines 22-23) whenWITH_ASANis enabled. While duplicate definitions don't cause build failures, this redundancy could lead to maintenance confusion if one location is updated but not the other.Consider removing these from one location to maintain a single source of truth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
store_handler/eloq_data_store_service/CMakeLists.txt(2 hunks)store_handler/eloq_data_store_service/build_eloq_store.cmake(2 hunks)
🔇 Additional comments (2)
store_handler/eloq_data_store_service/build_eloq_store.cmake (1)
120-120: LGTM!The linkage correctly uses the new
${BOOST_CONTEXT_TARGET}variable, which abstracts the Boost.Context dependency for both ASAN and non-ASAN builds.store_handler/eloq_data_store_service/CMakeLists.txt (1)
353-356: LGTM!The conditional block correctly uses
${BOOST_CONTEXT_TARGET}which is defined by the includedbuild_eloq_store.cmakewhenWITH_DATA_STOREisELOQDSS_ELOQSTORE. The guard condition ensures the variable is always defined when this code path executes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
store_handler/eloq_data_store_service/build_eloq_store.cmake(2 hunks)
🔇 Additional comments (1)
store_handler/eloq_data_store_service/build_eloq_store.cmake (1)
121-121:BOOST_CONTEXT_TARGETabstraction is a nice simplification—verify it’s always non-empty in both branches.Linking via
${BOOST_CONTEXT_TARGET}is fine (target in non-ASAN, absolute library path in ASAN). Just ensure configuration fails early whenWITH_ASAN=ONand the library isn’t found (otherwise this line becomes a confusing link error later).
| if(WITH_ASAN) | ||
| message("build eloqstore with ASAN: ${WITH_ASAN}") | ||
| set(BOOST_CONTEXT_ASAN_PATH "$ENV{HOME}/boost_ucontext_asan") | ||
| # https://www.boost.org/doc/libs/master/libs/context/doc/html/context/stack/sanitizers.html | ||
| add_compile_definitions(BOOST_USE_ASAN) | ||
| add_compile_definitions(BOOST_USE_UCONTEXT) | ||
|
|
||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -fno-omit-frame-pointer") | ||
| find_library(Boost_CONTEXT_LIBRARY | ||
| NAMES boost_context | ||
| PATHS ${BOOST_CONTEXT_ASAN_PATH}/lib | ||
| NO_DEFAULT_PATH) | ||
| set(BOOST_CONTEXT_TARGET ${Boost_CONTEXT_LIBRARY}) | ||
| else () | ||
| find_package(Boost REQUIRED COMPONENTS context) | ||
| set(BOOST_CONTEXT_TARGET Boost::context) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "build_eloq_store.cmake" -type fRepository: eloqdata/tx_service
Length of output: 125
🏁 Script executed:
# Read the entire file to understand context
cat -n store_handler/eloq_data_store_service/build_eloq_store.cmakeRepository: eloqdata/tx_service
Length of output: 5714
🏁 Script executed:
# Check if there's any code before line 19 that might set Boost_INCLUDE_DIRS for ASAN
head -20 store_handler/eloq_data_store_service/build_eloq_store.cmake | tail -10Repository: eloqdata/tx_service
Length of output: 330
🏁 Script executed:
# Check if this file is included from a parent CMakeLists that might set Boost_INCLUDE_DIRS
find . -name "CMakeLists.txt" | xargs grep -l "build_eloq_store.cmake" 2>/dev/null | head -3Repository: eloqdata/tx_service
Length of output: 134
🏁 Script executed:
# Check the parent CMakeLists to see if Boost is set up there
find . -path "./store_handler/eloq_data_store_service" -prune -o -name "CMakeLists.txt" -type f -print | head -5Repository: eloqdata/tx_service
Length of output: 219
🏁 Script executed:
# Check what's in the store_handler CMakeLists
cat store_handler/eloq_data_store_service/CMakeLists.txtRepository: eloqdata/tx_service
Length of output: 16038
🏁 Script executed:
# Check root CMakeLists for Boost setup
head -100 CMakeLists.txtRepository: eloqdata/tx_service
Length of output: 5670
🏁 Script executed:
# Final verification: check if Boost_INCLUDE_DIRS is ever set before line 77 in build_eloq_store.cmake
# by searching for any find_package(Boost or Boost_INCLUDE_DIRS assignment before it
grep -n "Boost_INCLUDE_DIRS\|find_package.*Boost" store_handler/eloq_data_store_service/build_eloq_store.cmake | head -20Repository: eloqdata/tx_service
Length of output: 146
🏁 Script executed:
# Verify that line 77 unconditionally uses Boost_INCLUDE_DIRS without checking ASAN path first
sed -n '70,85p' store_handler/eloq_data_store_service/build_eloq_store.cmakeRepository: eloqdata/tx_service
Length of output: 542
Add missing Boost.Context resolution in ASAN branch and harden find_library checks.
Lines 19-35 lack error handling and header discovery:
find_library(Boost_CONTEXT_LIBRARY ...)at line 27 has no NOTFOUND guard, risking silent linkage failure.- ASAN branch never sets
Boost_INCLUDE_DIRS, but line 77 unconditionally uses it inELOQ_STORE_INCLUDE, causing compilation to fail with missing Boost headers. - Hard-coded
$ENV{HOME}/boost_ucontext_asan(line 21) is non-portable for CI and shared machines.
Suggested fix:
if(WITH_ASAN)
message("build eloqstore with ASAN: ${WITH_ASAN}")
- set(BOOST_CONTEXT_ASAN_PATH "$ENV{HOME}/boost_ucontext_asan")
+ set(BOOST_CONTEXT_ASAN_PATH "$ENV{HOME}/boost_ucontext_asan" CACHE PATH "Prefix of ASAN-built Boost (expects include/ and lib/)")
# https://www.boost.org/doc/libs/master/libs/context/doc/html/context/stack/sanitizers.html
add_compile_definitions(BOOST_USE_ASAN)
add_compile_definitions(BOOST_USE_UCONTEXT)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address -fno-omit-frame-pointer")
+ find_path(Boost_INCLUDE_DIRS
+ NAMES boost/config.hpp
+ PATHS ${BOOST_CONTEXT_ASAN_PATH}/include
+ NO_DEFAULT_PATH)
+ if (NOT Boost_INCLUDE_DIRS)
+ message(FATAL_ERROR "Fail to find Boost headers under ${BOOST_CONTEXT_ASAN_PATH}/include (required for WITH_ASAN)")
+ endif()
find_library(Boost_CONTEXT_LIBRARY
NAMES boost_context
PATHS ${BOOST_CONTEXT_ASAN_PATH}/lib
NO_DEFAULT_PATH)
+ if (NOT Boost_CONTEXT_LIBRARY)
+ message(FATAL_ERROR "Fail to find boost_context under ${BOOST_CONTEXT_ASAN_PATH}/lib (required for WITH_ASAN)")
+ endif()
set(BOOST_CONTEXT_TARGET ${Boost_CONTEXT_LIBRARY})
else ()
find_package(Boost REQUIRED COMPONENTS context)
set(BOOST_CONTEXT_TARGET Boost::context)
endif()Also move BOOST_USE_ASAN, BOOST_USE_UCONTEXT, and sanitizer flags from global scope to target-level (via target_compile_definitions, target_compile_options, target_link_options) once eloqstore target is created, ensuring the final link step reliably gets -fsanitize=address without contaminating unrelated targets.
🤖 Prompt for AI Agents
In store_handler/eloq_data_store_service/build_eloq_store.cmake around lines 19
to 35, the ASAN branch currently hardcodes an ENV path, doesn't resolve Boost
include dirs, and doesn't guard against find_library failing; update it to (1)
allow an overridable ASAN boost path via a CMake variable (with the prior ENV
value as a default) instead of hardcoding $ENV{HOME}, (2) use find_path to
locate Boost_INCLUDE_DIRS (or derive include from the found library path) and
set Boost_INCLUDE_DIRS when the ASAN build is chosen so ELOQ_STORE_INCLUDE is
valid, (3) check the result of find_library and call message(FATAL_ERROR ...)
(or message(STATUS)+return) if the library was not found to avoid silent linkage
failure, and (4) remove global add_compile_definitions and global
CMAKE_CXX_FLAGS changes and instead plan to apply
BOOST_USE_ASAN/BOOST_USE_UCONTEXT and sanitizer compiler/link flags with
target_compile_definitions/target_compile_options/target_link_options on the
eloqstore target after it is created.
2 participants
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.