diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 5528d6442e..16385a01db 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -60,6 +60,11 @@ def get_cmake_args(build_type: str, extra_args: str) -> str: # Every config must declare 'minimal'. Minimal configs form the reduced matrix # built for pull requests by default; the full matrix adds the rest. # +# A Linux config may instead declare 'extended'. Neither the minimal nor the full +# matrix includes such a config; only the extended matrix does, which the nightly +# schedule and a manual run ask for. Use it for a config too expensive to run per +# pull request. It is Linux only because nothing else needs it yet. +# # Configs may also opt into 'benchmark' to smoke-run the benchmarks, or carry a # 'package' map to be packaged as well. Note that either applies to every entry # a config expands into, so only set them on configs that expand to a single @@ -94,6 +99,7 @@ class LinuxConfig: build_type: list[str] arch: list[str] minimal: bool + extended: bool = False benchmark: bool = False # if true, smoke-run the benchmarks after testing sanitizers: list[str] = dataclasses.field(default_factory=list) suffix: str = "" @@ -215,12 +221,22 @@ _ARCHS: dict[str, Architecture] = { } -def expand_linux_matrix(linux: LinuxFile, minimal: bool) -> list[MatrixEntry]: +def expand_linux_matrix( + linux: LinuxFile, minimal: bool, extended: bool = False +) -> list[MatrixEntry]: """Expand a LinuxFile into a flat list of matrix entries. Each config entry is expanded over the cross-product of its compiler, build_type, sanitizers, and architecture lists. When 'minimal' is true, only configs flagged as minimal are included. + + @param linux The parsed linux.json. + @param minimal Emit only the configs flagged 'minimal'. + @param extended Emit the configs flagged 'extended'. Both the minimal and + the full matrix leave those out, so this is the only way to get them. + @return One entry per combination the surviving configs expand into. + @note Do not set both 'minimal' and 'extended'. The command line rejects the + pair, but a direct caller gets the minimal matrix rather than an error. """ entries: list[MatrixEntry] = [] @@ -228,6 +244,8 @@ def expand_linux_matrix(linux: LinuxFile, minimal: bool) -> list[MatrixEntry]: for cfg in configs: if minimal and not cfg.minimal: continue + if not extended and cfg.extended: + continue # An empty sanitizers list means "one entry with no sanitizer". effective_sanitizers = cfg.sanitizers or [""] effective_archs = {arch: _ARCHS[arch] for arch in cfg.arch} @@ -367,7 +385,12 @@ if __name__ == "__main__": help="Emit the Linux packaging matrix instead of the build/test matrix.", action="store_true", ) - parser.add_argument( + # Each flag picks a matrix size, and the sizes nest: minimal is a subset of + # the full matrix, which is a subset of extended. So one flag narrows and the + # other widens the same default, and asking for both has no answer. Argparse + # rejects the pair rather than letting the filters intersect into a surprise. + size = parser.add_mutually_exclusive_group() + size.add_argument( "-m", "--minimal", help="Emit only the minimal matrix (the configs flagged 'minimal'), " @@ -375,6 +398,14 @@ if __name__ == "__main__": "emitted.", action="store_true", ) + size.add_argument( + "-x", + "--extended", + help="Emit the extended matrix: the full one plus the configs flagged " + "'extended', which no other matrix includes. Used for the nightly " + "schedule and for a manual run.", + action="store_true", + ) args = parser.parse_args() matrix: list[MatrixEntry] | list[PackagingEntry] = [] @@ -388,7 +419,7 @@ if __name__ == "__main__": else: if args.config in ("linux", None): matrix += expand_linux_matrix( - LinuxFile.load(THIS_DIR / "linux.json"), args.minimal + LinuxFile.load(THIS_DIR / "linux.json"), args.minimal, args.extended ) if args.config in ("macos", None): matrix += expand_platform_matrix( diff --git a/.github/scripts/strategy-matrix/linux.json b/.github/scripts/strategy-matrix/linux.json index 9b069f0ce3..ed3fe75cf7 100644 --- a/.github/scripts/strategy-matrix/linux.json +++ b/.github/scripts/strategy-matrix/linux.json @@ -38,6 +38,14 @@ "minimal": false, "sanitizers": ["address", "undefinedbehavior"] }, + { + "compiler": ["clang"], + "build_type": ["Debug"], + "arch": ["amd64"], + "minimal": false, + "extended": true, + "sanitizers": ["thread"] + }, { "compiler": ["clang"], diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 2a0c9332f3..2a67ae3c0c 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -117,6 +117,12 @@ jobs: VOIDSTAR_ENABLED: ${{ contains(inputs.cmake_args, '-Dvoidstar=ON') }} VALIDATOR_KEYS_ENABLED: ${{ contains(inputs.cmake_args, '-Dvalidator_keys=ON') }} SANITIZERS_ENABLED: ${{ inputs.sanitizers != '' }} + # One flag per sanitizer, so that a step needing only one of them does not + # have to parse the list. conan/profiles/sanitizers rejects any name that is + # not one of these three, so an unrecognized one never reaches a step. + ASAN_ENABLED: ${{ contains(inputs.sanitizers, 'address') }} + TSAN_ENABLED: ${{ contains(inputs.sanitizers, 'thread') }} + UBSAN_ENABLED: ${{ contains(inputs.sanitizers, 'undefinedbehavior') }} # The binaries reusable-package.yml consumes. A private repository skips # them except on a tag push, which is what produces its release packages. PACKAGING_ARTIFACTS_ENABLED: ${{ github.event.repository.visibility == 'public' || startsWith(github.ref, 'refs/tags/') }} @@ -221,7 +227,12 @@ jobs: ASAN_OPTS="${ASAN_OPTS}:alloc_dealloc_mismatch=0" fi echo "ASAN_OPTIONS=${ASAN_OPTS}" >>${GITHUB_ENV} - echo "TSAN_OPTIONS=include=${SUPP}/runtime-tsan-options.txt:suppressions=${SUPP}/tsan.supp" >>${GITHUB_ENV} + # exitcode=0 is set here rather than in runtime-tsan-options.txt, + # because that file is also what the documented local command reads, + # and a local run must still exit nonzero on a finding. TSan exits 66 + # on its own once it has reported anything, which would fail the job. + TSAN_OPTS="include=${SUPP}/runtime-tsan-options.txt:suppressions=${SUPP}/tsan.supp" + echo "TSAN_OPTIONS=${TSAN_OPTS}:exitcode=0" >>${GITHUB_ENV} echo "UBSAN_OPTIONS=include=${SUPP}/runtime-ubsan-options.txt:suppressions=${SUPP}/ubsan.supp" >>${GITHUB_ENV} echo "LSAN_OPTIONS=include=${SUPP}/runtime-lsan-options.txt:suppressions=${SUPP}/lsan.supp" >>${GITHUB_ENV} @@ -329,11 +340,102 @@ jobs: exit 1 fi + # Catch a build configured without the instrumentation it was asked for, + # which would otherwise run every test, pass, and report nothing. + # + # A sanitizer is checked by the symbols the binary carries, because + # instrumented code calls into its runtime, so those references are present + # whether the runtime is linked statically or as a shared library. That is + # what proves the -fsanitize flags reached the compiler. The version string + # cannot: cmake sets the SANITIZERS macro separately from those flags, so it + # names a sanitizer in a binary carrying none. It is checked as well, on top + # rather than instead, because BuildInfo.cpp is what a bug report quotes and + # it should agree with the binary. + # + # Asking each runtime to print its flag list was tried and dropped: GCC's + # libubsan ignores UBSAN_OPTIONS=help=1, so it called an instrumented gcc + # UBSan build uninstrumented. + # + # voidstar is a linked library rather than instrumentation, so the version + # string is all there is to check for it. + # + # The version call's exit code is ignored on purpose. ASan leaves leak + # reporting on and UBSan sets halt_on_error, either of which can end it + # nonzero on a healthy binary. Only the text decides, and it is printed. - name: Verify presence of instrumentation (Linux) - if: ${{ runner.os == 'Linux' && env.VOIDSTAR_ENABLED == 'true' }} + if: ${{ runner.os == 'Linux' && (env.VOIDSTAR_ENABLED == 'true' || env.SANITIZERS_ENABLED == 'true') }} working-directory: ${{ env.BUILD_DIR }} run: | - ./xrpld --version | grep libvoidstar + version="$(./xrpld --version 2>&1 || true)" + echo "${version}" + status=0 + + named() { + if grep -q "$2" <<<"${version}"; then + echo "${1}: the version string names '${2}'." + else + echo "${1} is enabled, but the version string does not name '${2}'." + status=1 + fi + } + + if [ "${SANITIZERS_ENABLED}" = 'true' ]; then + # Whichever of these the image ships. Stopping here beats a check + # that quietly stops checking. + symbols="$(mktemp)" + if command -v nm >/dev/null 2>&1; then + nm ./xrpld >"${symbols}" 2>/dev/null || true + elif command -v readelf >/dev/null 2>&1; then + readelf -sW ./xrpld >"${symbols}" 2>/dev/null || true + elif command -v objdump >/dev/null 2>&1; then + objdump -t ./xrpld >"${symbols}" 2>/dev/null || true + else + echo 'Found none of nm, readelf or objdump, so the binary cannot be inspected.' + exit 1 + fi + if [ ! -s "${symbols}" ]; then + echo 'Read no symbols from ./xrpld, so the instrumentation cannot be confirmed.' + exit 1 + fi + + # Each pattern names the calls a sanitizer plants at an instrumented + # operation, rather than the ones that only prove its runtime is + # present. __asan_init and __tsan_init appear as soon as the flags + # reach the linker, so matching those would pass a build whose + # compile step never saw them. __tsan_func_entry is no better: it + # survives an ignorelist, while the read and write calls do not. + # + # How much this buys depends on how the runtime is linked, and both + # kinds are in the matrix. GCC links libasan and libubsan + # dynamically, so an instrumented call site is the only thing that + # can put these symbols in the binary. Clang links the TSan runtime + # statically, so the runtime's own definitions are there either way + # and this narrows to a check that the sanitizer reached the link. + # Nothing cheap distinguishes the two cases, and the flags come from + # one cmake variable, so losing only the compile half is unlikely. + instrumented() { + if grep -qE "$2" "${symbols}"; then + echo "${1}: the binary calls into its runtime, matching '${2}'." + else + echo "${1} is enabled, but no instrumented call site matches '${2}'." + status=1 + fi + } + + [ "${ASAN_ENABLED}" = 'true' ] && + instrumented address '__asan_report|__asan_memcpy|__asan_stack_malloc' + [ "${TSAN_ENABLED}" = 'true' ] && + instrumented thread '__tsan_read|__tsan_write|__tsan_atomic' + [ "${UBSAN_ENABLED}" = 'true' ] && + instrumented undefinedbehavior '__ubsan_handle' + fi + + [ "${VOIDSTAR_ENABLED}" = 'true' ] && named voidstar libvoidstar + [ "${ASAN_ENABLED}" = 'true' ] && named address ASAN + [ "${TSAN_ENABLED}" = 'true' ] && named thread TSAN + [ "${UBSAN_ENABLED}" = 'true' ] && named undefinedbehavior UBSAN + + exit "${status}" - name: Run the separate tests if: ${{ !inputs.build_only }} @@ -350,6 +452,16 @@ jobs: # Coverage builds are slower due to instrumentation; use fewer parallel jobs to avoid flakiness [ "$COVERAGE_ENABLED" = "true" ] && BUILD_NPROC=$((BUILD_NPROC - 2)) + # TSan keeps shadow memory for every byte a process touches, so each job + # costs several times what it does uninstrumented, and too many at once + # exhausts the machine. Cap the count rather than subtract from it, + # because the ceiling is total memory and does not scale with the core + # count. Raise it only against a run, and expect one of two failures: + # the OOM killer ends the process with code 137 before any suite + # finishes, or, worse, the runner loses contact with the server and the + # job stops with no log and no failing step. + [ "$TSAN_ENABLED" = "true" ] && BUILD_NPROC=4 + # The resolver/preload workaround is only correct for the ASan build: # a regular build doesn't hit the __dn_expand interceptor bug, and must # NOT have libasan injected. So only preload when xrpld is ASan-built. diff --git a/.github/workflows/reusable-strategy-matrix.yml b/.github/workflows/reusable-strategy-matrix.yml index 12f11b0fbe..6d1daea428 100644 --- a/.github/workflows/reusable-strategy-matrix.yml +++ b/.github/workflows/reusable-strategy-matrix.yml @@ -39,4 +39,9 @@ jobs: # labeled "Ready to merge" or "Full CI build". Any other event (merge # queue, push, schedule, manual dispatch) runs the full matrix. GENERATE_MINIMAL: ${{ (github.event_name == 'pull_request' && !contains(github.event.pull_request.labels.*.name, 'Ready to merge') && !contains(github.event.pull_request.labels.*.name, 'Full CI build')) && '--minimal' || '' }} - run: ./generate.py ${GENERATE_CONFIG} ${GENERATE_MINIMAL} >>"${GITHUB_OUTPUT}" + # Configs flagged "extended" in the JSON are too expensive to run per + # pull request, so only the nightly schedule adds them. A manual run + # counts as well, because otherwise the only way to exercise one is to + # wait for the night. + GENERATE_EXTENDED: ${{ (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') && '--extended' || '' }} + run: ./generate.py ${GENERATE_CONFIG} ${GENERATE_MINIMAL} ${GENERATE_EXTENDED} >>"${GITHUB_OUTPUT}" diff --git a/BUILD.md b/BUILD.md index e98d204d0b..7e053e338c 100644 --- a/BUILD.md +++ b/BUILD.md @@ -273,7 +273,7 @@ Example use with some cmake variables set: ``` cd .build conan install .. --output-folder . --build missing --settings build_type=Debug -cmake -DCMAKE_BUILD_TYPE=Debug -Dcoverage=ON -Dxrpld=ON -Dtests=ON -Dcoverage_test_parallelism=2 -Dcoverage_format=html-details -Dcoverage_extra_args="--json coverage.json" -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake .. +cmake -DCMAKE_BUILD_TYPE=Debug -Dcoverage=ON -Dxrpld=ON -Dtests=ON -Dcoverage_format=html-details -Dcoverage_extra_args="--json coverage.json" -DCMAKE_TOOLCHAIN_FILE:FILEPATH=build/generators/conan_toolchain.cmake .. cmake --build . --target coverage ``` diff --git a/cmake/XrplSanitizers.cmake b/cmake/XrplSanitizers.cmake index 893c880374..9d70680c31 100644 --- a/cmake/XrplSanitizers.cmake +++ b/cmake/XrplSanitizers.cmake @@ -77,19 +77,24 @@ if(is_clang) message(STATUS " Ignorelist: ${ignorelist_path}") endif() -# Define SANITIZERS macro for BuildInfo.cpp +# Define the SANITIZERS macro for BuildInfo.cpp, plus one of XRPL_ASAN, +# XRPL_TSAN and XRPL_UBSAN per active sanitizer, so that code can test for a +# specific one with #ifdef instead of parsing the dot-joined SANITIZERS string. set(sanitizers_list) if(SANITIZERS MATCHES "address") set(enable_asan ON) list(APPEND sanitizers_list "ASAN") + target_compile_definitions(common INTERFACE XRPL_ASAN) endif() if(SANITIZERS MATCHES "thread") set(enable_tsan ON) list(APPEND sanitizers_list "TSAN") + target_compile_definitions(common INTERFACE XRPL_TSAN) endif() if(SANITIZERS MATCHES "undefinedbehavior") set(enable_ubsan ON) list(APPEND sanitizers_list "UBSAN") + target_compile_definitions(common INTERFACE XRPL_UBSAN) endif() if(sanitizers_list) diff --git a/docs/build/sanitizers.md b/docs/build/sanitizers.md index 6e7284fd06..f9a6a02a47 100644 --- a/docs/build/sanitizers.md +++ b/docs/build/sanitizers.md @@ -99,6 +99,40 @@ export TSAN_OPTIONS="include=sanitizers/suppressions/runtime-tsan-options.txt:su More details [here](https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual). +> [!IMPORTANT] +> The `ubuntu-clang-debug-amd64-tsan` CI config runs TSan in the full matrix +> only, and reports nothing back. `runtime-tsan-options.txt` sets +> `halt_on_error=false`, so the run continues past a finding, and the workflow +> appends `exitcode=0` to `TSAN_OPTIONS`, so the finding does not fail the job. +> Both are needed: TSan exits 66 on its own once it has reported anything. Read +> the job log to see findings. A test that must fail on one has to run in its own +> step with `halt_on_error=1` and `exitcode=66`. +> +> `exitcode=0` lives in the workflow and not in `runtime-tsan-options.txt`, +> because the local command above reads that same file. A local run keeps the +> default nonzero exit, so a finding fails the command. `exitcode=0` also only +> covers TSan's own exit path: a test that fails on its own still fails the job. +> +> Reporting nothing back is a first stage, not the end state. Once the set of +> findings the job produces is known and stable, drop `exitcode=0` from the +> workflow so that a new finding fails the job, and suppress what is left in +> third-party code. + +> [!IMPORTANT] +> Run TSan on Linux to check lock order. Linux reports an inversion as +> `WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock)`, with no +> `TSAN_OPTIONS` needed. macOS arm64 reports nothing, not even a genuine double +> lock of a non-recursive mutex, and not with `detect_deadlocks=1` or raw +> `pthread_mutex_t` either. The report strings are present in its runtime and the +> flag defaults to true, so that is a platform limit rather than a configuration +> mistake. Data race detection does work on macOS. + +> [!TIP] +> The build defines `XRPL_TSAN` when TSan is active, and `XRPL_ASAN` and +> `XRPL_UBSAN` for the other two. Use them to skip a test that only means +> something under one sanitizer. Skip at run time rather than with `#ifdef` +> around the body, so that every build still compiles the test. + ### LeakSanitizer (LSan) LSan is automatically enabled with ASAN. To disable it: @@ -150,15 +184,39 @@ More details [here](https://clang.llvm.org/docs/undefinedbehaviorSanitizer.html) ### [`tsan.supp`](../../sanitizers/suppressions/tsan.supp) -- **Purpose**: Suppress ThreadSanitizer data race warnings -- **Format**: `race:` where pattern matches function/file names +- **Purpose**: Suppress ThreadSanitizer warnings +- **Format**: `:` where pattern matches function/file names, and + type is `race`, `deadlock`, `signal`, `mutex` or `called_from_lib` - **More info**: [ThreadSanitizer suppressions](https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions) +- **Note**: Every `deadlock:` pattern must name a source file. One that names a + locking primitive instead, such as `pthread_rwlock_rdlock`, turns lock-order + checking off for every lock of that kind in the tree, which for that example is + every `std::shared_mutex` read lock. Suppress the file that reports the + inversion instead. A pattern of any type that names a file which has since + moved matches nothing, so check the paths when a subsystem is relocated. ### [`sanitizer-ignorelist.txt`](../../sanitizers/suppressions/sanitizer-ignorelist.txt) - **Purpose**: Compile-time ignorelist for all sanitizers - **Usage**: Passed via `-fsanitize-ignorelist=absolute/path/to/sanitizer-ignorelist.txt` -- **Format**: `:` (e.g., `src:Workers.cpp`) +- **Format**: `:` (e.g. `src:*Workers.cpp`) +- **Note**: This file is not a suppressions file, and the syntax differs. Clang + looks up only the entities `src`, `fun`, `global`, `type` and `mainfile`. A + `race:`, `deadlock:` or `signal:` entry parses without an error and is then + never consulted, so it does nothing; those types belong in `tsan.supp`. +- **Note**: The glob must match the whole path as the compiler receives it, which + the build makes absolute. So `src:core/detail/Workers.cpp` matches nothing, + while `src:*core/detail/Workers.cpp` matches. +- **Note**: To confirm that an entry works, compile the file with and without the + entry and compare how many times the object references the calls the sanitizer + plants at an instrumented operation: `__tsan_read` and `__tsan_write`, + `__asan_report`, or `__ubsan_handle`. For example + `nm -u .o | grep -cE '__tsan_(read|write)'`. A working entry lowers the + count, usually to zero, though code inlined from a header still counts because + a `src:` glob matches the file a function is defined in. Do not look for + `__tsan_func_entry`, which survives an ignorelist, or `__tsan_init`, which only + shows the runtime is present; either one reports an ignored file as + instrumented. ## Troubleshooting diff --git a/sanitizers/suppressions/runtime-tsan-options.txt b/sanitizers/suppressions/runtime-tsan-options.txt index aafe997ea4..3b2ce21398 100644 --- a/sanitizers/suppressions/runtime-tsan-options.txt +++ b/sanitizers/suppressions/runtime-tsan-options.txt @@ -1,3 +1,4 @@ halt_on_error=false verbosity=1 +detect_deadlocks=1 second_deadlock_stack=1 diff --git a/sanitizers/suppressions/sanitizer-ignorelist.txt b/sanitizers/suppressions/sanitizer-ignorelist.txt index ffd9b4103a..a3bd079418 100644 --- a/sanitizers/suppressions/sanitizer-ignorelist.txt +++ b/sanitizers/suppressions/sanitizer-ignorelist.txt @@ -1,32 +1,23 @@ -# We were seeing some false positives and some repeated errors(since these are library files) in following files. -# Clang will skip instrumenting the files added here. -# We should fix the underlying issues(if any) and remove these entries. - -deadlock:libxrpl/beast/utility/beast_Journal.cpp -deadlock:libxrpl/beast/utility/beast_PropertyStream.cpp -deadlock:test/beast/beast_PropertyStream_test.cpp -deadlock:xrpld/core/detail/Workers.cpp -deadlock:xrpld/core/JobQueue.cpp - -race:libxrpl/beast/utility/beast_Journal.cpp -race:libxrpl/beast/utility/beast_PropertyStream.cpp -race:test/beast/beast_PropertyStream_test.cpp -race:xrpld/core/detail/Workers.cpp -race:xrpld/core/JobQueue.cpp - -signal:libxrpl/beast/utility/beast_Journal.cpp -signal:libxrpl/beast/utility/beast_PropertyStream.cpp -signal:test/beast/beast_PropertyStream_test.cpp -signal:xrpld/core/detail/Workers.cpp -signal:xrpld/core/JobQueue.cpp - -src:beast/utility/beast_Journal.cpp -src:beast/utility/beast_PropertyStream.cpp -src:core/detail/Workers.cpp -src:core/JobQueue.cpp -src:libxrpl/beast/utility/beast_Journal.cpp -src:test/beast/beast_PropertyStream_test.cpp -src:src/test/app/Invariants_test.cpp +# Clang skips instrumenting the files listed here. Keep the list as short as it +# can be: an entry here removes a file from every sanitizer's view, so it hides +# real findings along with the false positive it was added for. +# +# This is a clang ignorelist, not a runtime suppressions file, and the two take +# different syntax. Here an entry reads :, and the only entities +# clang looks up are src, fun, global, type and mainfile. A `race:`, `deadlock:` +# or `signal:` entry parses without an error and is then never consulted, so it +# does nothing; those types belong in tsan.supp. The glob has to match the whole +# path as the compiler receives it, which the build makes absolute, so an entry +# needs a leading `*` to match anything at all. See docs/build/sanitizers.md. +# +# This list once held seven `src:` entries for beast_Journal.cpp, +# beast_PropertyStream.cpp, Workers.cpp, JobQueue.cpp, the PropertyStream test +# and Invariants_test.cpp, added against false positives. None carried the +# leading `*`, so none ever matched, and every one of those files has been +# instrumented the whole time. The sanitizer jobs pass that way, so the +# suppressions are not needed; they are removed rather than repaired. Restore one +# only with a finding that shows it is needed, and check it works by the method +# in sanitizers.md. # ASan false positive: stack-use-after-scope in ErrorCodes.h inline functions. # When Clang inlines the StaticString overloads (e.g. invalidFieldError(StaticString)), diff --git a/sanitizers/suppressions/tsan.supp b/sanitizers/suppressions/tsan.supp index 702eef1e3d..4a872625c5 100644 --- a/sanitizers/suppressions/tsan.supp +++ b/sanitizers/suppressions/tsan.supp @@ -35,43 +35,52 @@ race:src/xrpld/app/main/BasicApp.cpp race:src/xrpld/app/main/GRPCServer.cpp race:src/xrpld/app/misc/detail/AmendmentTable.cpp race:src/xrpld/app/misc/FeeVoteImpl.cpp -race:src/xrpld/app/rdb/detail/Wallet.cpp +race:src/libxrpl/server/Wallet.cpp race:src/xrpld/overlay/detail/OverlayImpl.cpp -race:src/xrpld/peerfinder/detail/PeerfinderManager.cpp -race:src/xrpld/peerfinder/detail/SourceStrings.cpp +race:src/libxrpl/peerfinder/PeerfinderManager.cpp +race:src/libxrpl/peerfinder/SourceStrings.cpp race:src/xrpld/rpc/detail/ServerHandler.cpp race:xrpl/server/detail/Door.h race:xrpl/server/detail/Spawn.h race:xrpl/server/detail/ServerImpl.h race:xrpl/nodestore/detail/DatabaseNodeImp.h race:src/libxrpl/beast/utility/beast_Journal.cpp -race:src/test/beast/LexicalCast_test.cpp +race:src/tests/libxrpl/beast/LexicalCast.cpp race:ServerHandler # More suppressions in external library code. race:crtstuff.c race:pipe -# Deadlock / lock-order-inversion suppressions -# Note: GCC's TSAN may not fully support all deadlock suppression patterns -deadlock:src/libxrpl/beast/utility/beast_Journal.cpp -deadlock:src/libxrpl/beast/utility/beast_PropertyStream.cpp -deadlock:src/test/beast/beast_PropertyStream_test.cpp -deadlock:src/xrpld/core/detail/Workers.cpp -deadlock:src/xrpld/app/misc/detail/Manifest.cpp -deadlock:src/xrpld/app/misc/detail/ValidatorList.cpp -deadlock:src/xrpld/app/misc/detail/ValidatorSite.cpp +# Deadlock / lock-order-inversion suppressions: deliberately none. +# +# The TSan job exists to find lock-order inversions, and it cannot fail a build, +# so a suppression here costs a finding and buys nothing. Ten used to sit below: +# +# - pthread_create, pthread_rwlock_rdlock and boost::asio, which name locking +# primitives rather than source files. Each of those turns lock-order +# checking off for every lock of that kind in the tree, which for +# pthread_rwlock_rdlock is every std::shared_mutex read lock, and between +# them they covered any inversion reached through a strand or a thread start. +# - ValidatorList.cpp and ValidatorSite.cpp. +# - beast_Journal.cpp, beast_PropertyStream.cpp, the PropertyStream test, +# Workers.cpp and Manifest.cpp. +# +# Manifest.cpp was the expensive one. ManifestCache::save() invokes the caller's +# predicate while holding its own lock, and the validator list takes the pair the +# other way, so the two deadlock. Removing that one line surfaced the cycle 52 +# times in a single run, on the shutdown path. +# +# If one has to come back, it must name a source file, and check that the file is +# where the pattern says: a suppression naming a moved file protects nothing. +# Note also that GCC's TSAN may not fully support all deadlock suppression +# patterns. signal:src/libxrpl/beast/utility/beast_Journal.cpp -signal:src/xrpld/core/detail/Workers.cpp -signal:src/xrpld/core/JobQueue.cpp +signal:src/libxrpl/core/detail/Workers.cpp +signal:src/libxrpl/core/detail/JobQueue.cpp signal:Workers::Worker -# Aggressive suppressing of deadlock tsan errors -deadlock:pthread_create -deadlock:pthread_rwlock_rdlock -deadlock:boost::asio - # Suppress SEGV crashes in TSAN itself during stringbuf operations # This appears to be a GCC-15 TSAN instrumentation issue with basic_stringbuf::str() # Commonly triggered in beast::Journal::ScopedStream destructor