build: add runtime sanitizer support and integration test wrapper helper - #22225
Draft
da-phil wants to merge 8 commits into
Draft
Conversation
Add DT_SANITIZE, a CMake option taking a comma separated list of address, undefined, leak and thread. It applies the instrumentation as compile/link options rather than as a build type, so it composes with RelWithDebInfo and does not collide with rawspeed's own Sanitize/TSan build types. Invalid combinations and MSan are rejected at configure time, and the runtime is link-checked so a missing libasan fails early. build.sh gains --sanitize; --asan is kept as an alias. The old --asan passed CFLAGS/LDFLAGS as environment prefixes to cmake, which CMake only consumes on the first configure into an empty cache, so re-running it on an existing build dir silently produced an uninstrumented binary. Drop -Werror for sanitizer builds: instrumentation perturbs the optimizer and produces fresh -Wmaybe-uninitialized / -Wstringop-overflow diagnostics that would otherwise abort the build. Add tools/run-integration-tests.sh and tools/sanitizer/aggregate-reports.py, which drive src/tests/integration from outside the submodule. They supply the per-invocation timeout the suite lacks, and rescue the sanitizer reports the suite would otherwise discard via its "2> /dev/null": log_path covers the runtimes reporting through sanitizer_common, and a darktable-cli shim captures stderr, where GCC's UBSan prints its non-fatal diagnostics regardless of log_path.
Both were found by AddressSanitizer during an integration run, and both are decidable without executing anything: the sizes involved are all in the DWARF of a build with debug info. check-iop-pipe-data-sizes.py: a module without its own init_pipe() gets default_init_pipe(), which allocates piece->data as calloc(1, params_size). That is only sound while the data struct is no larger than the params struct, and nothing enforces it. check-iop-legacy-params.py: a module's old parameter layouts survive as dt_iop_<op>_params_v<N>_t inside legacy_params(), and nothing checks they still match what those darktable versions wrote. Ground truth comes from the history entries in the integration test XMPs. A size mismatch is an error; a version with no matching struct is a warning, since the compiler need not emit a local type it had no use for. Neither invariant can be a static_assert: params_size is a runtime field and the data and legacy structs are module-local, so the two sides are never visible to the compiler together. Both read DWARF through tools/dwarf_types.py, which uses objdump rather than gdb, gdb resolves types through a lookup scope and cannot reach the function-local legacy typedefs.
GCC's libgomp carries no TSan annotations, so TSan cannot see the happens-before edges OpenMP barriers establish and reports a race on nearly every parallel loop: 97% of the reports in a full-suite run. The tsan.supp entries cannot catch these - called_from_lib only applies to interceptors, and race: needs symbolized names that a stripped libgomp does not have. Option --no-openmp runs the CLI with OMP_NUM_THREADS=1 instead, removing them at the source and leaving darktable's own threading visible. Distributions set DEBUGINFOD_URLS globally (/etc/debuginfod), which makes llvm-symbolizer block on network lookups -- 45s per module here against 0.02s with it cleared. That is why the runtimes gave up and printed unsymbolized "<null>" frames for the dlopen'd iop plugins, which are the frames worth reading. Clear it in the generated environment. aggregate-reports.py resolves any that still arrive as module+offset, given --build-dir. It runs after deduplication, on the frames about to be printed: a full-suite run is gigabytes of reports collapsing to a few hundred findings, so resolving during parsing would symbolize the same addresses tens of thousands of times.
Setting OMP_NUM_THREADS had no effect: dt_init() derives its thread count from -t/--threads and then calls omp_set_num_threads() (src/common/darktable.c:1618), overriding the environment. The suite always passes -t 4, so the flag was silently ignored. Rewrite -t to 1 in the darktable-cli shim instead, which owns the argument list. The environment variable stays for the OpenMP users that do honour it, rawspeed among them. On 0000-nop under ThreadSanitizer: 326s to 17s, 1.3MB of reports to 88KB, 119 unique findings to 18.
All four *_OPTIONS share sanitizer_common's log_path, so whichever set is parsed last decides where reports land -- and which that is differs by runtime. A GCC address,undefined build wrote its ASan reports to ubsan.<pid>, and a clang thread-only build wrote ThreadSanitizer reports to ubsan-<pass>.<pid>. Both send you looking in the wrong file. Gate each block on DT_SANITIZERS so the file name says what the report is. LSAN_OPTIONS is kept for 'address' as well, since ASan embeds LeakSanitizer.
Contributor
Author
|
I'm pushing those changes in case somebody wants to play while I'm on vacation the next 2 weeks, have fun ;) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Run darktable's integration tests under sanitizers
Adds the build support, a test driver, and two static checks derived from what the sanitizers found.
Build
DT_SANITIZEtakes a comma-separated list ofaddress,undefined,leak,thread.It is applied as compile/link options, so it composes with
RelWithDebInfoand does not collide with rawspeed's ownSanitize/TSanbuild types.Invalid combinations and MSan are rejected at configure time and the runtime is link-checked, so a missing
libasanfails early.build.shgains--sanitize;--asanstays as an alias.ASan is built with
-fsanitize-recover=addressand run withhalt_on_error=0so one run collects every finding instead of stopping at whichever comes first.Each runtime's
*_OPTIONSis exported only when that sanitizer is built in.All four share
sanitizer_common'slog_path, so whichever set is parsed last decides where reports land, and which that is differs by runtime: a GCCaddress,undefinedbuild wrote ASan reports toubsan.<pid>, and a clangthreadbuild wrote ThreadSanitizer reports toubsan-<pass>.<pid>.DEBUGINFOD_URLSis cleared. Distributions set it globally, and llvm-symbolizer then blocks on network lookups for modules it cannot satisfy locally - 45s per module against 0.02s with it cleared.Driver
tools/run-integration-tests.shdrivessrc/tests/integrationfrom outside the submodule, without patching anything in it. The suite runs darktable-cli as$* 1> /dev/null 2> /dev/null, so every sanitizer report would otherwise be discarded.It:
log_pathfor the runtimes that honour it and a stderr-capturing shim for GCC's UBSan, which ignores itmain()and every test then fails identically in a fraction of a second, which reads like a darktable bug. On kernels defaultingvm.mmap_rnd_bitsto 32 this affects TSan, and the driver falls back tosetarch -Ron its own--no-openmp. GCC's libgomp carries no TSan annotations, so TSan cannot see the happens-before edges OpenMP barriers establish and reports a race on nearly every parallel loop: 97% of the findings in a full-suite run. Thetsan.suppentries cannot catch these -called_from_libonly applies to interceptors, andrace:needs symbolized names a stripped libgomp does not have. Serialising removes them at the source. It rewrites the suite's-trather than settingOMP_NUM_THREADS, becausedt_init()derives its thread count from-tand then callsomp_set_num_threads(), overriding the environment. On0000-nopunder TSan: 326s to 17s, 1.3MB of reports to 88KB, 119 unique findings to 18.tools/sanitizer/aggregate-reports.pydeduplicates the findings and names the tests each one appeared in.Given
--build-dirit also resolves the frames the runtimes leave asmodule+offset: TSan symbolizes the binary and its link-time libraries but not the dlopen'd iop plugins, which are the frames worth reading.That runs after deduplication, on the frames about to be printed - a full-suite run is gigabytes of reports collapsing to a few hundred findings.
Static checks
Two invariants the sanitizers found the hard way are decidable without running anything, because both sides are in the DWARF of a build with debug info:
tools/check-iop-pipe-data-sizes.py- a module without its owninit_pipe()getsdefault_init_pipe(), which allocatespiece->dataascalloc(1, params_size). Sound only while the data struct is no larger than the params struct.tools/check-iop-legacy-params.py-dt_iop_<op>_params_v<N>_tstructs must still match what those darktable versions wrote. Ground truth is the history entries in the integration test XMPs.Neither can be a
static_assert:params_sizeis a runtime field and the data and legacy structs are module-local, so the two sides are never visible to the compiler together.Both read DWARF through
tools/dwarf_types.py, which usesobjdump- gdb cannot reach the function-local legacy typedefs - so they need only binutils and anyRelWithDebInfotree.How to run it - Quickstart
Here is an example with ASAN and UBSAN instrumentation:
What it found
Three heap-buffer-overflows, each fixed in its own PR:
Still open: a lock-order-inversion between
dt_cache_get_with_caller(cache.c:150,:233) anddt_rawspeed_load_meta(imageio_rawspeed.cc:68), closed bydt_variables_expandre-entering the cache.It reproduces in every test under GCC with and without OpenMP, and under clang, so it is not a runtime artifact.
Here are example log outputs of running all integration tests with sanitizer instrumentation:
Disclaimer: this change was co-created with Claude.