fix: Gate -mcmodel flags to x86_64 in sanitizer builds - #7049
Conversation
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
pratikmankawde
added
the
DraftRunCI
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
pratikmankawde
removed
the
DraftRunCI
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7049 +/- ##
=======================================
Coverage 82.1% 82.1%
=======================================
Files 1010 1010
Lines 75994 75994
Branches 7388 7377 -11
=======================================
+ Hits 62355 62357 +2
+ Misses 13639 13637 -2 see 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| if(is_amd64) | ||
| message(STATUS " Using medium code model (-mcmodel=medium)") | ||
| list(APPEND SANITIZERS_COMPILE_FLAGS "-mcmodel=medium") | ||
| list(APPEND SANITIZERS_RELOCATION_FLAGS "-mcmodel=medium") | ||
| endif() |
There was a problem hiding this comment.
In the profile I now see {% set model_code = "-mcmodel=large" %} or {% set model_code = "-mcmodel=medium" %} only for x64 depending on whether address or thread sanitization is requested.
In these if-else statements, why is the mcmodel also explicitly being set? It feels like duplication. Can't the model_code set in the profile be made available here so you can just append it to SANITIZERS_COMPILE_FLAGS and SANITIZERS_RELOCATION_FLAGS, if set?
There was a problem hiding this comment.
The profile sets the flags for all dependencies, and then the flags set in XrplSanitizers.cmake are for xrpld.
I think we have had a discussion about de-duplicating this setup before. That can be done in a dedicated initiative.
There was a problem hiding this comment.
We did, and since I keep noticing it let's spend some time on deduplication.
| if(is_amd64) | ||
| message(STATUS " Using medium code model (-mcmodel=medium)") | ||
| list(APPEND SANITIZERS_COMPILE_FLAGS "-mcmodel=medium") | ||
| list(APPEND SANITIZERS_RELOCATION_FLAGS "-mcmodel=medium") | ||
| endif() |
There was a problem hiding this comment.
We did, and since I keep noticing it let's spend some time on deduplication.
pratikmankawde
added
the
Ready to merge
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
3 participants
High Level Overview of Change
Gate
-mcmodel=large(ASAN) and-mcmodel=medium(TSAN) behind x86_64 architecture checks in both the Conan sanitizer profile and CMake sanitizer module, fixing ARM64 ASAN/TSAN CI builds.Context of Change
The
debian-bookworm-gcc-15-arm64-debug-asanCI job fails during the Conan dependency build phase (snappy) with:https://github.com/XRPLF/rippled/actions/runs/25019997726/job/73277846482
The CMake module (
XrplSanitizers.cmake) already had anis_amd64guard on the ASAN path but not on the TSAN path. The Conan profile (conan/profiles/sanitizers) had no architecture check at all, causing the flags to propagate to all dependency builds on all architectures.Testing
Testing on CI by running build with config:
debian-bookworm-gcc-15-arm64-debug-asanWorking build: https://github.com/XRPLF/rippled/actions/runs/25121180695/job/73622146413?pr=7049