From 7097566d9dea17ebd676e64a9a54f097a4de2231 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Fri, 28 Nov 2025 13:15:34 +0000 Subject: [PATCH] code review comments Signed-off-by: Pratik Mankawde --- .github/scripts/strategy-matrix/generate.py | 78 +++++++++---------- .../workflows/reusable-build-test-config.yml | 2 +- BUILD.md | 2 +- conan/profiles/sanitizers | 52 ++++++------- 4 files changed, 60 insertions(+), 74 deletions(-) diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 078e76b819..b930359c33 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -247,17 +247,33 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: "gcc-15", "clang-20", }: - addSanitizerConfigs( - architecture, - os, - build_type, - cmake_args, - config_name, - build_only, - cmake_target, - cxx_flags, - configurations, - ) + configs = addSanitizerConfigs(architecture, os, cmake_args, cxx_flags) + if "asan_ubsan" in configs: + configurations.append( + { + "config_name": config_name + "asan_ubsan", + "cmake_args": configs["asan_ubsan"], + "cmake_target": cmake_target, + "build_only": build_only, + "build_type": build_type, + "os": os, + "architecture": architecture, + "sanitizers": "Address", + } + ) + if "tsan_ubsan" in configs: + configurations.append( + { + "config_name": config_name + "tsan_ubsan", + "cmake_args": configs["tsan_ubsan"], + "cmake_target": cmake_target, + "build_only": build_only, + "build_type": build_type, + "os": os, + "architecture": architecture, + "sanitizers": "Thread", + } + ) else: if cxx_flags: cmake_args_flags = f"{cmake_args} -DCMAKE_CXX_FLAGS={cxx_flags}" @@ -281,14 +297,9 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: def addSanitizerConfigs( architecture: dict, os: dict, - build_type: str, cmake_args: str, - config_name: str, - build_only: bool, - cmake_target: str, cxx_flags: str, - configurations: list[dict], -): +) -> dict: extra_warning_flags = "" linker_relocation_flags = "" linker_flags = "" @@ -336,19 +347,10 @@ def addSanitizerConfigs( # First create config for asan cmake_args_flags = f'{cmake_args} -DCMAKE_CXX_FLAGS="-fsanitize=address,{sanitizers_flags} -fno-omit-frame-pointer {cxx_flags} {extra_warning_flags}" {linker_flags}' - # Add config with asan - configurations.append( - { - "config_name": config_name + "_asan", - "cmake_args": cmake_args_flags, - "cmake_target": cmake_target, - "build_only": build_only, - "build_type": build_type, - "os": os, - "architecture": architecture, - "sanitizers": "Address", - } - ) + # Add config with asan+ubsan + configs = {} + configs["asan_ubsan"] = cmake_args_flags + # Since TSAN runs are crashing with seg faults(could be compatibility issues with latest compilers) # We deactivate it for now. But I would keep the code, since it took some effort to find the correct set of config needed to run this. # This will be useful when we decide to activate it again in future. @@ -377,18 +379,10 @@ def addSanitizerConfigs( cmake_args_flags = f"{cmake_args} -DCMAKE_CXX_FLAGS='-fsanitize=thread,{sanitizers_flags} -fno-omit-frame-pointer {cxx_flags} {extra_warning_flags}' {linker_flags}" - configurations.append( - { - "config_name": config_name + "_tsan", - "cmake_args": cmake_args_flags, - "cmake_target": cmake_target, - "build_only": build_only, - "build_type": build_type, - "os": os, - "architecture": architecture, - "sanitizers": "Thread", - } - ) + # Add config with tsan+ubsan + configs["tsan_ubsan"] = cmake_args_flags + + return configs def read_config(file: Path) -> Config: diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index a2af717917..69a59573e7 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -54,7 +54,7 @@ on: description: "The sanitizers to enable ('Address+UndefinedBehaviour' or 'Thread+UndefinedBehaviour')." required: false type: string - default: "None" + default: "" secrets: CODECOV_TOKEN: diff --git a/BUILD.md b/BUILD.md index 52bb315fe1..b1b6390506 100644 --- a/BUILD.md +++ b/BUILD.md @@ -373,7 +373,7 @@ tools.build:cxxflags=['-DBOOST_ASIO_DISABLE_CONCEPTS'] Command: `conan cache clean "*"` ``` - SANITIZERS=Address conan install .. --output-folder . --profile sanitizers --build missing --settings build_type=Debug + SANITIZERS=Address conan install .. --output-folder . --profile:all sanitizers --build missing --settings build_type=Debug ``` Available options for SANITIZERS: `Address` and `Thread` diff --git a/conan/profiles/sanitizers b/conan/profiles/sanitizers index d08861bb1b..a1db42b511 100644 --- a/conan/profiles/sanitizers +++ b/conan/profiles/sanitizers @@ -4,38 +4,30 @@ include(default) {% set sanitizers = os.getenv("SANITIZERS") %} [conf] - {% if compiler == "gcc" %} -{% set asan_sanitizer_flags = "-fsanitize=address,"~default_sanitizer_flags~" -mcmodel=large -fno-PIC" %} -{% set tsan_sanitizer_flags = "-fsanitize=thread,"~default_sanitizer_flags~" -mcmodel=medium -fno-PIC" %} - -{% if sanitizers == "Address" %} -tools.build:cxxflags+=['{{asan_sanitizer_flags}} -fno-omit-frame-pointer -O1 -Wno-stringop-overflow'] -tools.build:sharedlinkflags+=['{{asan_sanitizer_flags}}'] -tools.build:exelinkflags+=['{{asan_sanitizer_flags}}'] - -{% elif sanitizers == "Thread" %} -tools.build:cxxflags+=['{{tsan_sanitizer_flags}} -fno-omit-frame-pointer -O1 -Wno-stringop-overflow -Wno-tsan'] -tools.build:sharedlinkflags+=['{{tsan_sanitizer_flags}}'] -tools.build:exelinkflags+=['{{tsan_sanitizer_flags}}'] - -{% endif %} - + {% set asan_sanitizer_flags = "-fsanitize=address,"~default_sanitizer_flags~" -mcmodel=large -fno-PIC" %} + {% set tsan_sanitizer_flags = "-fsanitize=thread,"~default_sanitizer_flags~" -mcmodel=medium -fno-PIC" %} + {% if sanitizers == "Address" %} + tools.build:cxxflags+=['{{asan_sanitizer_flags}} -fno-omit-frame-pointer -O1 -Wno-stringop-overflow'] + tools.build:sharedlinkflags+=['{{asan_sanitizer_flags}}'] + tools.build:exelinkflags+=['{{asan_sanitizer_flags}}'] + {% elif sanitizers == "Thread" %} + tools.build:cxxflags+=['{{tsan_sanitizer_flags}} -fno-omit-frame-pointer -O1 -Wno-stringop-overflow -Wno-tsan'] + tools.build:sharedlinkflags+=['{{tsan_sanitizer_flags}}'] + tools.build:exelinkflags+=['{{tsan_sanitizer_flags}}'] + {% endif %} {% elif compiler == "apple-clang" or compiler == "clang" %} -{% set asan_sanitizer_flags = "-fsanitize=address,"~default_sanitizer_flags~",unsigned-integer-overflow -fno-PIC" %} -{% set tsan_sanitizer_flags = "-fsanitize=thread,"~default_sanitizer_flags~",unsigned-integer-overflow -fno-PIC" %} -{% if sanitizers == "Address" %} -tools.build:cxxflags+=['{{asan_sanitizer_flags}} -fno-omit-frame-pointer -O1'] -tools.build:sharedlinkflags+=['{{asan_sanitizer_flags}}'] -tools.build:exelinkflags+=['{{asan_sanitizer_flags}}'] - -{% elif sanitizers == "Thread" %} -tools.build:cxxflags+=['{{tsan_sanitizer_flags}} -fno-omit-frame-pointer -O1'] -tools.build:sharedlinkflags+=['{{tsan_sanitizer_flags}}'] -tools.build:exelinkflags+=['{{tsan_sanitizer_flags}}'] - -{% endif %} - + {% set asan_sanitizer_flags = "-fsanitize=address,"~default_sanitizer_flags~",unsigned-integer-overflow -fno-PIC" %} + {% set tsan_sanitizer_flags = "-fsanitize=thread,"~default_sanitizer_flags~",unsigned-integer-overflow -fno-PIC" %} + {% if sanitizers == "Address" %} + tools.build:cxxflags+=['{{asan_sanitizer_flags}} -fno-omit-frame-pointer -O1'] + tools.build:sharedlinkflags+=['{{asan_sanitizer_flags}}'] + tools.build:exelinkflags+=['{{asan_sanitizer_flags}}'] + {% elif sanitizers == "Thread" %} + tools.build:cxxflags+=['{{tsan_sanitizer_flags}} -fno-omit-frame-pointer -O1'] + tools.build:sharedlinkflags+=['{{tsan_sanitizer_flags}}'] + tools.build:exelinkflags+=['{{tsan_sanitizer_flags}}'] + {% endif %} {% endif %} tools.info.package_id:confs+=["tools.build:cxxflags", "tools.build:exelinkflags", "tools.build:sharedlinkflags"]