code review changes

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
2026-06-03 08:46:46 +00:00 · 2025-12-16 11:57:04 +00:00
parent 93b0554a36
commit 32f7baca44
4 changed files with 68 additions and 72 deletions
--- a/BUILD.md
+++ b/BUILD.md
@@ -389,13 +389,13 @@ tools.build:cxxflags=['-DBOOST_ASIO_DISABLE_CONCEPTS']
   ```

   **Sanitizers:** To build dependencies with sanitizer instrumentation, set the
-   `SANITIZERS` environment variable and use the `sanitizers` profile:
+   `SANITIZERS` environment variable(only once for both conan and cmake) and use the `sanitizers` profile:

   ```
-   SANITIZERS=Address,UndefinedBehavior conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
+   export 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.
   See [sanitizers documentation](./docs/build/sanitizers.md) for more details.

   To build Debug, in the next step, be sure to set `-DCMAKE_BUILD_TYPE=Debug`
--- a/cmake/XrplSanitizers.cmake
+++ b/cmake/XrplSanitizers.cmake
@@ -28,14 +28,28 @@ set(ENABLE_ASAN FALSE)
 set(ENABLE_TSAN FALSE)
 set(ENABLE_UBSAN FALSE)

-if(SANITIZERS MATCHES "Address")
+# Normalize SANITIZERS into a list
+set(_san_list "${SANITIZERS}")
+string(REPLACE "," ";" _san_list "${_san_list}")
+separate_arguments(_san_list)
+
+set(REMAINING_SANITIZERS "")
+foreach(_san IN LISTS _san_list)
+  if(_san STREQUAL "Address")
    set(ENABLE_ASAN TRUE)
-endif()
-if(SANITIZERS MATCHES "Thread")
+  elseif(_san STREQUAL "Thread")
    set(ENABLE_TSAN TRUE)
-endif()
-if(SANITIZERS MATCHES "UndefinedBehavior")
+  elseif(_san STREQUAL "UndefinedBehavior")
    set(ENABLE_UBSAN TRUE)
+  else()
+    list(APPEND REMAINING_SANITIZERS "${_san}")
+  endif()
+endforeach()
+
+if(REMAINING_SANITIZERS)
+  message(FATAL_ERROR
+    "Unknown SANITIZERS value(s): ${REMAINING_SANITIZERS}. "
+    "Supported: Address, Thread, UndefinedBehavior and their combinations.")
 endif()

 # Detect compiler type
@@ -98,6 +112,13 @@ endif()

 # Compiler-specific configuration
 if(IS_GCC)
+    # Remove any -fuse-ld=... flags previously added to 'common'
+    get_target_property(_common_if_libs common INTERFACE_LINK_LIBRARIES)
+    if(_common_if_libs)
+        list(REMOVE_ITEM _common_if_libs "-fuse-ld=mold" "-fuse-ld=gold" "-fuse-ld=lld")
+        set_target_properties(common PROPERTIES INTERFACE_LINK_LIBRARIES "${_common_if_libs}")
+    endif()
+
    # 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)
--- a/docs/build/sanitizers.md
+++ b/docs/build/sanitizers.md
@@ -8,14 +8,11 @@ Corresponding suppression files are located in the `sanitizers/suppressions` dir
    - [Summary](#summary)
    - [Build steps:](#build-steps)
      - [Install dependencies](#install-dependencies)
-      - [AddressSanitizer (ASan) + UndefinedBehaviorSanitizer (UBSan)](#addresssanitizer-asan--undefinedbehaviorsanitizer-ubsan)
-      - [ThreadSanitizer (TSan) + UndefinedBehaviorSanitizer (UBSan)](#threadsanitizer-tsan--undefinedbehaviorsanitizer-ubsan)
-      - [Just AddressSanitizer (ASan)](#just-addresssanitizer-asan)
-      - [Just UndefinedBehaviorSanitizer (UBSan)](#just-undefinedbehaviorsanitizer-ubsan)
+      - [Call CMake](#call-cmake)
      - [Build](#build)
  - [Running Tests with Sanitizers](#running-tests-with-sanitizers)
    - [AddressSanitizer (ASan)](#addresssanitizer-asan)
-    - [ThreadSanitizer (TSan) + UndefinedBehaviorSanitizer (UBSan)](#threadsanitizer-tsan--undefinedbehaviorsanitizer-ubsan-1)
+    - [ThreadSanitizer (TSan) + UndefinedBehaviorSanitizer (UBSan)](#threadsanitizer-tsan--undefinedbehaviorsanitizer-ubsan)
    - [LeakSanitizer (LSan)](#leaksanitizer-lsan)
  - [Suppression Files](#suppression-files)
    - [`asan.supp`](#asansupp)
@@ -35,8 +32,9 @@ 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 `SANITIZERS` environment variable when running CMake to enable sanitizers.
-3. Optionally use `--profile sanitizers` with Conan to build dependencies with sanitizer instrumentation.
+2. Set the `SANITIZERS` environment variable before calling conan install and cmake. Only set it once. Make sure both conan and cmake read the same values.
+   Example: `export SANITIZERS=Address,UndefinedBehavior`
+3. Optionally use `--profile:all 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.

 ---
@@ -55,53 +53,27 @@ cd .build
 The `SANITIZERS` environment variable is used by both Conan and CMake.

 ```bash
+export SANITIZERS=Address,UndefinedBehavior
 # Standard build (without instrumenting dependencies)
-SANITIZERS=Address,UndefinedBehavior conan install .. --output-folder . --build missing --settings build_type=Debug
+conan install .. --output-folder . --build missing --settings build_type=Debug

 # Or with sanitizer-instrumented dependencies (takes longer but fewer false positives)
-SANITIZERS=Address,UndefinedBehavior conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
-```
-
-#### AddressSanitizer (ASan) + UndefinedBehaviorSanitizer (UBSan)
-
-Set the `SANITIZERS` environment variable when running CMake:
-
-```bash
-SANITIZERS=Address,UndefinedBehavior cmake .. -G Ninja \
-    -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-    -DCMAKE_BUILD_TYPE=Debug \
-    -Dtests=ON -Dxrpld=ON
-```
-
-#### ThreadSanitizer (TSan) + UndefinedBehaviorSanitizer (UBSan)
-
-```bash
-SANITIZERS=Thread,UndefinedBehavior cmake .. -G Ninja \
-    -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-    -DCMAKE_BUILD_TYPE=Debug \
-    -Dtests=ON -Dxrpld=ON
-```
-
-#### Just AddressSanitizer (ASan)
-
-```bash
-SANITIZERS=Address cmake .. -G Ninja \
-    -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-    -DCMAKE_BUILD_TYPE=Debug \
-    -Dtests=ON -Dxrpld=ON
-```
-
-#### Just UndefinedBehaviorSanitizer (UBSan)
-
-```bash
-SANITIZERS=UndefinedBehavior cmake .. -G Ninja \
-    -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
-    -DCMAKE_BUILD_TYPE=Debug \
-    -Dtests=ON -Dxrpld=ON
+conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
 ```

 **Note:** Do not mix Address and Thread sanitizers - they are incompatible.

+Since you already set the `SANITIZERS` environment variable when running Conan, same values will be read for the next part.
+
+#### Call CMake
+
+```bash
+cmake .. -G Ninja \
+    -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake \
+    -DCMAKE_BUILD_TYPE=Debug \
+    -Dtests=ON -Dxrpld=ON
+```
+
 #### Build

 ```bash
--- a/sanitizers/suppressions/ubsan.supp
+++ b/sanitizers/suppressions/ubsan.supp
@@ -199,36 +199,39 @@ unsigned-integer-overflow:__charconv/traits.h

 # Suppress errors in RocksDB
 # RocksDB uses intentional unsigned integer overflows in hash functions and CRC calculations
-unsigned-integer-overflow:rocks*/b/src/util/xxhash.h
-unsigned-integer-overflow:rocks*/b/src/util/xxph3.h
-unsigned-integer-overflow:rocks*/b/src/util/hash.cc
-unsigned-integer-overflow:rocks*/b/src/util/crc32c.cc
-unsigned-integer-overflow:rocks*/b/src/util/crc32c.h
-unsigned-integer-overflow:rocks*/b/src/include/rocksdb/utilities/options_type.h
-unsigned-integer-overflow:rocks*/b/src/table/format.h
-unsigned-integer-overflow:rocks*/b/src/table/format.cc
-unsigned-integer-overflow:rocks*/b/src/table/block_based/block_based_table_builder.cc
-unsigned-integer-overflow:rocks*/b/src/table/block_based/reader_common.cc
-unsigned-integer-overflow:rocks*/b/src/db/version_set.cc
+unsigned-integer-overflow:rocks*/*/util/xxhash.h
+unsigned-integer-overflow:rocks*/*/util/xxph3.h
+unsigned-integer-overflow:rocks*/*/util/hash.cc
+unsigned-integer-overflow:rocks*/*/util/crc32c.cc
+unsigned-integer-overflow:rocks*/*/util/crc32c.h
+unsigned-integer-overflow:rocks*/*/include/rocksdb/utilities/options_type.h
+unsigned-integer-overflow:rocks*/*/table/format.h
+unsigned-integer-overflow:rocks*/*/table/format.cc
+unsigned-integer-overflow:rocks*/*/table/block_based/block_based_table_builder.cc
+unsigned-integer-overflow:rocks*/*/table/block_based/reader_common.cc
+unsigned-integer-overflow:rocks*/*/db/version_set.cc

 # RocksDB misaligned loads (intentional for performance on ARM64)
-alignment:rocks*/b/src/util/crc32c_arm64.cc
+alignment:rocks*/*/util/crc32c_arm64.cc

 # nudb intentional overflows in hash functions
 unsigned-integer-overflow:nudb/detail/xxhash.hpp
 alignment:nudb/detail/xxhash.hpp

 # Snappy compression library intentional overflows
-unsigned-integer-overflow:snapp*/b/src/snappy.cc
+unsigned-integer-overflow:snappy.cc

 # Abseil intentional overflows
-unsigned-integer-overflow:abse*/b/src/absl/strings/numbers.cc
-unsigned-integer-overflow:abse*/b/src/absl/strings/internal/cord_rep_flat.h
+unsigned-integer-overflow:absl/strings/numbers.cc
+unsigned-integer-overflow:absl/strings/internal/cord_rep_flat.h
+unsigned-integer-overflow:absl/base/internal/low_level_alloc.cc
+unsigned-integer-overflow:absl/hash/internal/hash.h
+unsigned-integer-overflow:absl/container/internal/raw_hash_set.h

 # Standard library intentional overflows in chrono duration arithmetic
 unsigned-integer-overflow:__chrono/duration.h

 # Suppress undefined errors in RocksDB and nudb
-undefined:rocks.*/b/src/util/crc32c_arm64.cc
-undefined:rocks.*/b/src/util/xxhash.h
+undefined:rocks.*/*/util/crc32c_arm64.cc
+undefined:rocks.*/*/util/xxhash.h
 undefined:nudb