code review comments

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
This commit is contained in:
Pratik Mankawde
2025-12-15 17:34:17 +00:00
parent c8e3ba5d90
commit 3cd4e5cb5a
4 changed files with 151 additions and 164 deletions

View File

@@ -389,10 +389,10 @@ tools.build:cxxflags=['-DBOOST_ASIO_DISABLE_CONCEPTS']
```
**Sanitizers:** To build dependencies with sanitizer instrumentation, set the
`SANITIZER` environment variable and use the `sanitizers` profile:
`SANITIZERS` environment variable and use the `sanitizers` profile:
```
SANITIZER=Address,UndefinedBehavior conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
SANITIZERS=Address,UndefinedBehavior conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
```
Note: Do not mix Address and Thread sanitizers - they are incompatible.
@@ -435,10 +435,10 @@ tools.build:cxxflags=['-DBOOST_ASIO_DISABLE_CONCEPTS']
**Note:** You can pass build options for `xrpld` in this step.
**Sanitizers:** To enable sanitizers (Address, Thread, UndefinedBehavior),
set the `SANITIZER` environment variable when running CMake:
set the `SANITIZERS` environment variable when running CMake:
```
SANITIZER=Address,UndefinedBehavior cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Debug -Dxrpld=ON -Dtests=ON ..
SANITIZERS=Address,UndefinedBehavior cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Debug -Dxrpld=ON -Dtests=ON ..
```
See [sanitizers documentation](./docs/build/sanitizers.md) for more details.
@@ -549,7 +549,7 @@ stored inside the build directory, as either of:
| `werr` | OFF | Treat compilation warnings as errors |
| `wextra` | OFF | Enable additional compilation warnings |
To enable sanitizers, set the `SANITIZER` environment variable when running CMake.
To enable sanitizers, set the `SANITIZERS` environment variable when running CMake.
See [sanitizers documentation](./docs/build/sanitizers.md) for details.
[Unity builds][5] may be faster for the first build

View File

@@ -2,7 +2,7 @@
Configure sanitizers based on environment variables.
This module reads the following environment variables:
- SANITIZER: The sanitizers to enable. Possible values:
- SANITIZERS: The sanitizers to enable. Possible values:
- "Address"
- "Address,UndefinedBehavior"
- "Thread"
@@ -14,145 +14,139 @@
#]===================================================================]
# Read environment variable
set(SANITIZER $ENV{SANITIZER})
if(SANITIZER)
message(STATUS "Configuring sanitizers: ${SANITIZER}")
# Parse SANITIZER value to determine which sanitizers to enable
set(ENABLE_ASAN FALSE)
set(ENABLE_TSAN FALSE)
set(ENABLE_UBSAN FALSE)
if(SANITIZER MATCHES "Address")
set(ENABLE_ASAN TRUE)
endif()
if(SANITIZER MATCHES "Thread")
set(ENABLE_TSAN TRUE)
endif()
if(SANITIZER MATCHES "UndefinedBehavior")
set(ENABLE_UBSAN TRUE)
endif()
# Detect compiler type
set(IS_GCC FALSE)
set(IS_CLANG FALSE)
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(IS_GCC TRUE)
message(STATUS " Compiler: GCC ${CMAKE_CXX_COMPILER_VERSION}")
elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
set(IS_CLANG TRUE)
message(STATUS " Compiler: Clang ${CMAKE_CXX_COMPILER_VERSION}")
endif()
# Detect platform (amd64/x86_64 vs arm64/aarch64)
set(IS_AMD64 FALSE)
if(CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64|AMD64")
set(IS_AMD64 TRUE)
message(STATUS " Platform: amd64")
else()
message(STATUS " Platform: ${CMAKE_SYSTEM_PROCESSOR}")
endif()
# Frame pointer is required for meaningful stack traces
set(SANITIZER_COMPILE_FLAGS "-fno-omit-frame-pointer")
# Sanitizers recommend minimum of -O1 for reasonable performance
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -O1")
# Build the sanitizer flags string
set(SANITIZER_FLAGS "")
if(ENABLE_ASAN)
set(SANITIZER_FLAGS "address")
elseif(ENABLE_TSAN)
set(SANITIZER_FLAGS "thread")
endif()
if(ENABLE_UBSAN)
# UB sanitizer flags
if(IS_CLANG)
# Clang supports additional UB checks
set(UBSAN_FLAGS "undefined,float-divide-by-zero,unsigned-integer-overflow")
else()
set(UBSAN_FLAGS "undefined,float-divide-by-zero")
endif()
if(SANITIZER_FLAGS)
set(SANITIZER_FLAGS "${SANITIZER_FLAGS},${UBSAN_FLAGS}")
else()
set(SANITIZER_FLAGS "${UBSAN_FLAGS}")
endif()
endif()
# Configure code model for GCC on amd64
# Use large code model for ASAN to avoid relocation errors
# Use medium code model for TSAN (large is not compatible with TSAN)
set(SANITIZER_RELOCATION_FLAGS "")
if(IS_GCC AND IS_AMD64)
if(ENABLE_ASAN)
message(STATUS " Using large code model (-mcmodel=large)")
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -mcmodel=large")
set(SANITIZER_RELOCATION_FLAGS "-mcmodel=large")
elseif(ENABLE_TSAN)
message(STATUS " Using medium code model (-mcmodel=medium)")
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -mcmodel=medium")
set(SANITIZER_RELOCATION_FLAGS "-mcmodel=medium")
endif()
endif()
# Compiler-specific configuration
if(IS_GCC)
# Disable mold, gold and lld linkers for GCC with sanitizers
# Use default linker (bfd/ld) which is more lenient with mixed code models
set(use_mold OFF CACHE BOOL "Use mold linker" FORCE)
set(use_gold OFF CACHE BOOL "Use gold linker" FORCE)
set(use_lld OFF CACHE BOOL "Use lld linker" FORCE)
message(STATUS " Disabled mold, gold, and lld linkers for GCC with sanitizers")
# Suppress false positive warnings in GCC with stringop-overflow
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -Wno-stringop-overflow")
if(ENABLE_TSAN)
# GCC doesn't support atomic_thread_fence with tsan. Suppress warnings.
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -Wno-tsan")
endif()
# Add sanitizer to compile and link flags
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -fsanitize=${SANITIZER_FLAGS}")
set(SANITIZER_LINK_FLAGS "${SANITIZER_RELOCATION_FLAGS} -fsanitize=${SANITIZER_FLAGS}")
elseif(IS_CLANG)
# Add ignorelist for Clang (GCC doesn't support this)
# Use CMAKE_SOURCE_DIR to get the path to the ignorelist
set(IGNORELIST_PATH "${CMAKE_SOURCE_DIR}/sanitizers/suppressions/sanitizer-ignorelist.txt")
if(EXISTS "${IGNORELIST_PATH}")
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -fsanitize-ignorelist=${IGNORELIST_PATH}")
message(STATUS " Using sanitizer ignorelist: ${IGNORELIST_PATH}")
else()
message(WARNING "Sanitizer ignorelist not found: ${IGNORELIST_PATH}")
endif()
# Add sanitizer to compile and link flags
set(SANITIZER_COMPILE_FLAGS "${SANITIZER_COMPILE_FLAGS} -fsanitize=${SANITIZER_FLAGS}")
set(SANITIZER_LINK_FLAGS "-fsanitize=${SANITIZER_FLAGS}")
endif()
message(STATUS " Compile flags: ${SANITIZER_COMPILE_FLAGS}")
message(STATUS " Link flags: ${SANITIZER_LINK_FLAGS}")
# Convert space-separated strings to lists for CMake
separate_arguments(SANITIZER_COMPILE_FLAGS_LIST NATIVE_COMMAND "${SANITIZER_COMPILE_FLAGS}")
separate_arguments(SANITIZER_LINK_FLAGS_LIST NATIVE_COMMAND "${SANITIZER_LINK_FLAGS}")
# Apply the sanitizer flags to the 'common' interface library
# This is the same library used by XrplCompiler.cmake
target_compile_options(common INTERFACE
$<$<COMPILE_LANGUAGE:CXX>:${SANITIZER_COMPILE_FLAGS_LIST}>
$<$<COMPILE_LANGUAGE:C>:${SANITIZER_COMPILE_FLAGS_LIST}>
)
# Apply linker flags
target_link_options(common INTERFACE ${SANITIZER_LINK_FLAGS_LIST})
set(SANITIZERS $ENV{SANITIZERS})
if(NOT SANITIZERS)
return()
endif()
message(STATUS "Configuring sanitizers: ${SANITIZERS}")
# Parse SANITIZERS value to determine which sanitizers to enable
set(ENABLE_ASAN FALSE)
set(ENABLE_TSAN FALSE)
set(ENABLE_UBSAN FALSE)
if(SANITIZERS MATCHES "Address")
set(ENABLE_ASAN TRUE)
endif()
if(SANITIZERS MATCHES "Thread")
set(ENABLE_TSAN TRUE)
endif()
if(SANITIZERS MATCHES "UndefinedBehavior")
set(ENABLE_UBSAN TRUE)
endif()
# Detect compiler type
set(IS_GCC FALSE)
set(IS_CLANG FALSE)
if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
set(IS_GCC TRUE)
message(STATUS " Compiler: GCC ${CMAKE_CXX_COMPILER_VERSION}")
elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
set(IS_CLANG TRUE)
message(STATUS " Compiler: Clang ${CMAKE_CXX_COMPILER_VERSION}")
endif()
# Detect platform (amd64/x86_64 vs arm64/aarch64)
set(IS_AMD64 FALSE)
if(CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64|AMD64")
set(IS_AMD64 TRUE)
message(STATUS " Platform: amd64")
else()
message(STATUS " Platform: ${CMAKE_SYSTEM_PROCESSOR}")
endif()
# Frame pointer is required for meaningful stack traces. Sanitizers recommend minimum of -O1 for reasonable performance
set(SANITIZERS_COMPILE_FLAGS "-fno-omit-frame-pointer" "-O1")
# Build the sanitizer flags list
set(SANITIZERS_FLAGS)
if(ENABLE_ASAN)
list(APPEND SANITIZERS_FLAGS "address")
elseif(ENABLE_TSAN)
list(APPEND SANITIZERS_FLAGS "thread")
endif()
if(ENABLE_UBSAN)
# UB sanitizer flags
if(IS_CLANG)
# Clang supports additional UB checks
list(APPEND SANITIZERS_FLAGS "undefined" "float-divide-by-zero" "unsigned-integer-overflow")
else()
list(APPEND SANITIZERS_FLAGS "undefined" "float-divide-by-zero")
endif()
endif()
# Configure code model for GCC on amd64
# Use large code model for ASAN to avoid relocation errors
# Use medium code model for TSAN (large is not compatible with TSAN)
set(SANITIZERS_RELOCATION_FLAGS "")
if(IS_GCC AND IS_AMD64)
if(ENABLE_ASAN)
message(STATUS " Using large code model (-mcmodel=large)")
list(APPEND SANITIZERS_COMPILE_FLAGS "-mcmodel=large")
set(SANITIZERS_RELOCATION_FLAGS "-mcmodel=large")
elseif(ENABLE_TSAN)
message(STATUS " Using medium code model (-mcmodel=medium)")
list(APPEND SANITIZERS_COMPILE_FLAGS "-mcmodel=medium")
set(SANITIZERS_RELOCATION_FLAGS "-mcmodel=medium")
endif()
endif()
# Compiler-specific configuration
if(IS_GCC)
# Disable mold, gold and lld linkers for GCC with sanitizers
# Use default linker (bfd/ld) which is more lenient with mixed code models
set(use_mold OFF CACHE BOOL "Use mold linker" FORCE)
set(use_gold OFF CACHE BOOL "Use gold linker" FORCE)
set(use_lld OFF CACHE BOOL "Use lld linker" FORCE)
message(STATUS " Disabled mold, gold, and lld linkers for GCC with sanitizers")
# Suppress false positive warnings in GCC with stringop-overflow
list(APPEND SANITIZERS_COMPILE_FLAGS "-Wno-stringop-overflow")
if(ENABLE_TSAN)
# GCC doesn't support atomic_thread_fence with tsan. Suppress warnings.
list(APPEND SANITIZERS_COMPILE_FLAGS "-Wno-tsan")
endif()
# Join sanitizer flags with commas for -fsanitize option
list(JOIN SANITIZERS_FLAGS "," SANITIZERS_FLAGS_STR)
# Add sanitizer to compile and link flags
list(APPEND SANITIZERS_COMPILE_FLAGS "-fsanitize=${SANITIZERS_FLAGS_STR}")
set(SANITIZERS_LINK_FLAGS "${SANITIZERS_RELOCATION_FLAGS}" "-fsanitize=${SANITIZERS_FLAGS_STR}")
elseif(IS_CLANG)
# Add ignorelist for Clang (GCC doesn't support this)
# Use CMAKE_SOURCE_DIR to get the path to the ignorelist
set(IGNORELIST_PATH "${CMAKE_SOURCE_DIR}/sanitizers/suppressions/sanitizer-ignorelist.txt")
if(NOT EXISTS "${IGNORELIST_PATH}")
message(FATAL_ERROR "Sanitizer ignorelist not found: ${IGNORELIST_PATH}")
endif()
list(APPEND SANITIZERS_COMPILE_FLAGS "-fsanitize-ignorelist=${IGNORELIST_PATH}")
message(STATUS " Using sanitizer ignorelist: ${IGNORELIST_PATH}")
# Join sanitizer flags with commas for -fsanitize option
list(JOIN SANITIZERS_FLAGS "," SANITIZERS_FLAGS_STR)
# Add sanitizer to compile and link flags
list(APPEND SANITIZERS_COMPILE_FLAGS "-fsanitize=${SANITIZERS_FLAGS_STR}")
set(SANITIZERS_LINK_FLAGS "-fsanitize=${SANITIZERS_FLAGS_STR}")
endif()
message(STATUS " Compile flags: ${SANITIZERS_COMPILE_FLAGS}")
message(STATUS " Link flags: ${SANITIZERS_LINK_FLAGS}")
# Apply the sanitizer flags to the 'common' interface library
# This is the same library used by XrplCompiler.cmake
target_compile_options(common INTERFACE
$<$<COMPILE_LANGUAGE:CXX>:${SANITIZERS_COMPILE_FLAGS}>
$<$<COMPILE_LANGUAGE:C>:${SANITIZERS_COMPILE_FLAGS}>
)
# Apply linker flags
target_link_options(common INTERFACE ${SANITIZERS_LINK_FLAGS})

View File

@@ -19,20 +19,13 @@ include(default)
{% set extra_cxxflags = extra_cxxflags ~ " -Wno-tsan" %}
{% endif %}
{% if "UndefinedBehavior" in sanitizers or "UndefinedBehavior" in sanitizers %}
{% if "UndefinedBehavior" in sanitizers %}
{% set _ = sanitizer_list.append("undefined") %}
{% set _ = sanitizer_list.append("float-divide-by-zero") %}
{% endif %}
{% set sanitizer_flags = "-fsanitize=" ~ ",".join(sanitizer_list) ~ " " ~ model_code %}
{% if version >= "15" %}
# Note: GCC-15 has issues with Abseil where constexpr evaluation fails.
# Work around by disabling Abseil's V2 constant init logic
{% set extra_cxxflags = extra_cxxflags ~ " -DABSL_ENABLE_CONSTANT_INIT_V2=0" %}
abseil/*:tools.build:defines+=['ABSL_ENABLE_CONSTANT_INIT_V2=0']
{% endif %}
tools.build:cxxflags+=['{{sanitizer_flags}} {{extra_cxxflags}}']
tools.build:sharedlinkflags+=['{{sanitizer_flags}}']
tools.build:exelinkflags+=['{{sanitizer_flags}}']
@@ -63,4 +56,4 @@ include(default)
{% endif %}
{% endif %}
tools.info.package_id:confs+=["tools.build:cxxflags", "tools.build:exelinkflags", "tools.build:sharedlinkflags", "tools.build:defines", "tools.cmake.cmaketoolchain:user_toolchain"]
tools.info.package_id:confs+=["tools.build:cxxflags", "tools.build:exelinkflags", "tools.build:sharedlinkflags"]

View File

@@ -35,7 +35,7 @@ Corresponding suppression files are located in the `sanitizers/suppressions` dir
Follow the same instructions as mentioned in [BUILD.md](../../BUILD.md) but with the following changes:
1. Make sure you have a clean build directory.
2. Set the `SANITIZER` environment variable when running CMake to enable sanitizers.
2. Set the `SANITIZERS` environment variable when running CMake to enable sanitizers.
3. Optionally use `--profile sanitizers` with Conan to build dependencies with sanitizer instrumentation.
4. Set `ASAN_OPTIONS`, `LSAN_OPTIONS`, `UBSAN_OPTIONS` and `TSAN_OPTIONS` environment variables to configure sanitizer behavior when running executables.
@@ -52,22 +52,22 @@ cd .build
#### Install dependencies
The `SANITIZER` environment variable is used by both Conan and CMake.
The `SANITIZERS` environment variable is used by both Conan and CMake.
```bash
# Standard build (without instrumenting dependencies)
SANITIZER=Address,UndefinedBehavior conan install .. --output-folder . --build missing --settings build_type=Debug
SANITIZERS=Address,UndefinedBehavior conan install .. --output-folder . --build missing --settings build_type=Debug
# Or with sanitizer-instrumented dependencies (takes longer but fewer false positives)
SANITIZER=Address,UndefinedBehavior conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
SANITIZERS=Address,UndefinedBehavior conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
```
#### AddressSanitizer (ASan) + UndefinedBehaviorSanitizer (UBSan)
Set the `SANITIZER` environment variable when running CMake:
Set the `SANITIZERS` environment variable when running CMake:
```bash
SANITIZER=Address,UndefinedBehavior cmake .. -G Ninja \
SANITIZERS=Address,UndefinedBehavior cmake .. -G Ninja \
-DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-DCMAKE_BUILD_TYPE=Debug \
-Dtests=ON -Dxrpld=ON
@@ -76,7 +76,7 @@ SANITIZER=Address,UndefinedBehavior cmake .. -G Ninja \
#### ThreadSanitizer (TSan) + UndefinedBehaviorSanitizer (UBSan)
```bash
SANITIZER=Thread,UndefinedBehavior cmake .. -G Ninja \
SANITIZERS=Thread,UndefinedBehavior cmake .. -G Ninja \
-DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-DCMAKE_BUILD_TYPE=Debug \
-Dtests=ON -Dxrpld=ON
@@ -85,7 +85,7 @@ SANITIZER=Thread,UndefinedBehavior cmake .. -G Ninja \
#### Just AddressSanitizer (ASan)
```bash
SANITIZER=Address cmake .. -G Ninja \
SANITIZERS=Address cmake .. -G Ninja \
-DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-DCMAKE_BUILD_TYPE=Debug \
-Dtests=ON -Dxrpld=ON
@@ -94,7 +94,7 @@ SANITIZER=Address cmake .. -G Ninja \
#### Just UndefinedBehaviorSanitizer (UBSan)
```bash
SANITIZER=UndefinedBehavior cmake .. -G Ninja \
SANITIZERS=UndefinedBehavior cmake .. -G Ninja \
-DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-DCMAKE_BUILD_TYPE=Debug \
-Dtests=ON -Dxrpld=ON