adapt to boost ucontext for asan#193
Conversation
|
Warning Rate limit exceeded@thweetkomputer has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughUpdates the data_substrate submodule pointer and introduces ASAN-specific build configuration in CMake. When ASAN is enabled with coroutines, the build now sets a flag to use the ucontext backend and applies corresponding Boost compile definitions to the SQL target. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sql/CMakeLists.txt (1)
319-321: LGTM! The compile definitions are correctly scoped.The use of
PRIVATEscope for these Boost-specific definitions is appropriate since they're implementation details of the sql library. The placement after the target creation and linking is correct.Optional: Consider simplifying the conditional logic
The intermediate variable
SQL_USE_ASAN_UCONTEXTcould be eliminated for a more direct approach:- if (WITH_ASAN) - # Boost stackful contexts need to switch to the ucontext backend when - # ASAN is enabled, otherwise fcontext symbols are missing at link time. - set(SQL_USE_ASAN_UCONTEXT ON) - endif() + # Boost stackful contexts need to switch to the ucontext backend when + # ASAN is enabled, otherwise fcontext symbols are missing at link time.Then at lines 319-321:
-if (SQL_USE_ASAN_UCONTEXT) +if (COROUTINE_ENABLED AND WITH_ASAN) target_compile_definitions(sql PRIVATE BOOST_USE_ASAN BOOST_USE_UCONTEXT) endif()However, the current approach with the intermediate variable is also fine and may be clearer for future maintainers or if additional ASAN-related configuration is added later.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
data_substratesql/CMakeLists.txt
🔇 Additional comments (1)
sql/CMakeLists.txt (1)
288-292: The ASAN configuration is correct and follows Boost.Context official documentation. The macrosBOOST_USE_ASANandBOOST_USE_UCONTEXTare properly documented in the Boost.Context sanitizers support guide, and using compile definitions to force the ucontext backend (instead of the default fcontext assembly implementation) is the recommended approach to avoid linker issues with AddressSanitizer. TheWITH_ASANoption is properly defined in the project's root CMakeLists.txt. No changes needed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.