code review changes

Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
2026-04-29 15:37:57 +00:00 · 2025-12-18 16:58:49 +00:00
parent 1c10f730a1
commit 46d9937ffa
7 changed files with 48 additions and 55 deletions
--- a/BUILD.md
+++ b/BUILD.md
@@ -388,16 +388,6 @@ tools.build:cxxflags=['-DBOOST_ASIO_DISABLE_CONCEPTS']
   conan install .. --output-folder . --build missing --settings build_type=Debug
   ```

-   **Sanitizers:** To build dependencies with sanitizer instrumentation, set the
-   `SANITIZERS` environment variable(only once for both conan and cmake) and use the `sanitizers` profile:
-
-   ```
-   export SANITIZERS=Address,UndefinedBehavior
-   conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
-   ```
-
-   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`

   For a single-configuration generator, e.g. `Unix Makefiles` or `Ninja`,
@@ -434,15 +424,6 @@ 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 `SANITIZERS` environment variable when running CMake:
-
-   ```
-   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.
-
 4. Build `xrpld`.

   For a single-configuration generator, it will build whatever configuration
@@ -537,6 +518,21 @@ stored inside the build directory, as either of:
 - file named `coverage.`_extension_, with a suitable extension for the report format, or
 - directory named `coverage`, with the `index.html` and other files inside, for the `html-details` or `html-nested` report formats.

+## Sanitizers
+
+To build dependencies and xprld with sanitizer instrumentation, set the
+`SANITIZERS` environment variable(only once before running conan and cmake) and use the `sanitizers` profile in conan:
+
+```bash
+export SANITIZERS=Address,UndefinedBehavior
+
+conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug
+
+cmake -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake -DCMAKE_BUILD_TYPE=Debug -Dxrpld=ON -Dtests=ON ..
+```
+
+See [Sanitizers docs](./docs/build/sanitizers.md) for more details.
+
 ## Options

 | Option     | Default Value | Description                                                    |
@@ -549,9 +545,6 @@ 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 `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
 (at the cost of much more memory) since they concatenate sources into fewer
 translation units. Non-unity builds may be faster for incremental builds,
--- a/cmake/CompilationEnv.cmake
+++ b/cmake/CompilationEnv.cmake
@@ -62,6 +62,8 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64|AMD64")
  set(is_amd64 TRUE)
 elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64|arm64")
  set(is_arm64 TRUE)
+else()
+  message(FATAL_ERROR "Unknown architecture: ${CMAKE_SYSTEM_PROCESSOR}")
 endif()

 # Keep legacy uppercase alias for existing code
--- a/cmake/XrplCompiler.cmake
+++ b/cmake/XrplCompiler.cmake
@@ -121,8 +121,8 @@ else ()
      # link to static libc/c++ iff:
      #   * static option set and
      #   * NOT APPLE (AppleClang does not support static libc/c++) and
-      #   * NOT san (sanitizers typically don't work with static libc/c++)
-      $<$<AND:$<BOOL:${static}>,$<NOT:$<BOOL:${APPLE}>>,$<NOT:$<BOOL:${san}>>>:
+      #   * NOT SANITIZER (sanitizers typically don't work with static libc/c++)
+      $<$<AND:$<BOOL:${static}>,$<NOT:$<BOOL:${APPLE}>>,$<NOT:$<BOOL:${SANITIZER}>>>:
      -static-libstdc++
      -static-libgcc
      >)
--- a/cmake/XrplInterface.cmake
+++ b/cmake/XrplInterface.cmake
@@ -44,21 +44,7 @@ if(jemalloc)
  target_link_libraries(opts INTERFACE jemalloc::jemalloc)
 endif ()

-if (san)
-  target_compile_options (opts
-    INTERFACE
-      # sanitizers recommend minimum of -O1 for reasonable performance
-      $<$<CONFIG:Debug>:-O1>
-      ${SAN_FLAG}
-      -fno-omit-frame-pointer)
-  target_compile_definitions (opts
-    INTERFACE
-      $<$<STREQUAL:${san},address>:SANITIZER=ASAN>
-      $<$<STREQUAL:${san},thread>:SANITIZER=TSAN>
-      $<$<STREQUAL:${san},memory>:SANITIZER=MSAN>
-      $<$<STREQUAL:${san},undefined>:SANITIZER=UBSAN>)
-  target_link_libraries (opts INTERFACE ${SAN_FLAG} ${SAN_LIB})
-endif ()
+# Sanitizer configuration is handled in XrplSanitizers.cmake

 #[===================================================================[
   xrpld transitive library deps via an interface library
--- a/cmake/XrplSanitizers.cmake
+++ b/cmake/XrplSanitizers.cmake
@@ -64,7 +64,7 @@ endif()
 if(ENABLE_UBSAN)
    # UB sanitizer flags
    if(is_clang)
-        # Clang supports additional UB checks
+        # Clang supports additional UB checks. More info here https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html
        list(APPEND SANITIZERS_FLAGS "undefined" "float-divide-by-zero" "unsigned-integer-overflow")
    else()
        list(APPEND SANITIZERS_FLAGS "undefined" "float-divide-by-zero")
@@ -75,19 +75,8 @@ endif()
 # 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")
-        list(APPEND SANITIZERS_RELOCATION_FLAGS "-mcmodel=large")
-    elseif(ENABLE_TSAN)
-        message(STATUS "  Using medium code model (-mcmodel=medium)")
-        list(APPEND SANITIZERS_COMPILE_FLAGS "-mcmodel=medium")
-        list(APPEND SANITIZERS_RELOCATION_FLAGS "-mcmodel=medium")
-    endif()
-endif()

-	# Compiler-specific configuration
+# 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
@@ -99,9 +88,16 @@ if(is_gcc)
    # Suppress false positive warnings in GCC with stringop-overflow
    list(APPEND SANITIZERS_COMPILE_FLAGS "-Wno-stringop-overflow")

-    if(ENABLE_TSAN)
+    if(is_amd64 AND ENABLE_ASAN)
+        message(STATUS "  Using large code model (-mcmodel=large)")
+        list(APPEND SANITIZERS_COMPILE_FLAGS "-mcmodel=large")
+        list(APPEND SANITIZERS_RELOCATION_FLAGS "-mcmodel=large")
+    elseif(ENABLE_TSAN)
        # GCC doesn't support atomic_thread_fence with tsan. Suppress warnings.
        list(APPEND SANITIZERS_COMPILE_FLAGS "-Wno-tsan")
+         message(STATUS "  Using medium code model (-mcmodel=medium)")
+        list(APPEND SANITIZERS_COMPILE_FLAGS "-mcmodel=medium")
+        list(APPEND SANITIZERS_RELOCATION_FLAGS "-mcmodel=medium")
    endif()

    # Join sanitizer flags with commas for -fsanitize option
@@ -142,3 +138,12 @@ target_compile_options(common INTERFACE

 # Apply linker flags
 target_link_options(common INTERFACE ${SANITIZERS_LINK_FLAGS})
+
+# Define SANITIZER macro for BuildInfo.cpp
+if(ENABLE_ASAN)
+  target_compile_definitions(common INTERFACE SANITIZER=ASAN)
+elseif(ENABLE_TSAN)
+  target_compile_definitions(common INTERFACE SANITIZER=TSAN)
+elseif(ENABLE_UBSAN)
+  target_compile_definitions(common INTERFACE SANITIZER=UBSAN)
+endif()
--- a/cmake/XrplSettings.cmake
+++ b/cmake/XrplSettings.cmake
@@ -58,7 +58,14 @@ else()
  set(wextra OFF CACHE BOOL "gcc/clang only" FORCE)
 endif()

-if(is_linux AND NOT $ENV{SANITIZERS})
+# Sanitizer configuration from environment (flags set in XrplSanitizers.cmake)
+if($ENV{SANITIZERS})
+  set(SANITIZER TRUE)
+else()
+  set(SANITIZER FALSE)
+endif()
+
+if(is_linux AND NOT SANITIZER)
  option(BUILD_SHARED_LIBS "build shared xrpl libraries" OFF)
  option(static "link protobuf, openssl, libc++, and boost statically" ON)
  option(perf "Enables flags that assist with perf recording" OFF)
--- a/cmake/deps/Boost.cmake
+++ b/cmake/deps/Boost.cmake
@@ -32,7 +32,7 @@ target_link_libraries(xrpl_boost
 if(Boost_COMPILER)
  target_link_libraries(xrpl_boost INTERFACE Boost::disable_autolinking)
 endif()
-if(san AND is_clang)
+if(SANITIZER AND is_clang)
  # TODO: gcc does not support -fsanitize-blacklist...can we do something else
  # for gcc ?
  if(NOT Boost_INCLUDE_DIRS AND TARGET Boost::headers)