From 46d9937ffa401011a874b24117ffe5fd793fa67e Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 18 Dec 2025 16:58:49 +0000 Subject: [PATCH] code review changes Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> --- BUILD.md | 37 +++++++++++++++---------------------- cmake/CompilationEnv.cmake | 2 ++ cmake/XrplCompiler.cmake | 4 ++-- cmake/XrplInterface.cmake | 16 +--------------- cmake/XrplSanitizers.cmake | 33 +++++++++++++++++++-------------- cmake/XrplSettings.cmake | 9 ++++++++- cmake/deps/Boost.cmake | 2 +- 7 files changed, 48 insertions(+), 55 deletions(-) diff --git a/BUILD.md b/BUILD.md index d6beaeed64..a22611ff92 100644 --- 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, diff --git a/cmake/CompilationEnv.cmake b/cmake/CompilationEnv.cmake index 4c84f26de6..1a720468bb 100644 --- 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 diff --git a/cmake/XrplCompiler.cmake b/cmake/XrplCompiler.cmake index 923c6f4cae..6dc6f4424a 100644 --- 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++) - $<$,$>,$>>: + # * NOT SANITIZER (sanitizers typically don't work with static libc/c++) + $<$,$>,$>>: -static-libstdc++ -static-libgcc >) diff --git a/cmake/XrplInterface.cmake b/cmake/XrplInterface.cmake index 0a2f67ddf6..ff2c5aa473 100644 --- 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 - $<$:-O1> - ${SAN_FLAG} - -fno-omit-frame-pointer) - target_compile_definitions (opts - INTERFACE - $<$:SANITIZER=ASAN> - $<$:SANITIZER=TSAN> - $<$:SANITIZER=MSAN> - $<$: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 diff --git a/cmake/XrplSanitizers.cmake b/cmake/XrplSanitizers.cmake index 426d096ea9..86be20d76e 100644 --- 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() diff --git a/cmake/XrplSettings.cmake b/cmake/XrplSettings.cmake index 6e30703fb7..8b2ab421da 100644 --- 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) diff --git a/cmake/deps/Boost.cmake b/cmake/deps/Boost.cmake index 475c1033b2..d05e812e37 100644 --- 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)