From 52287c025f0c6574043956cac1ad2538d9683181 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Fri, 10 Jul 2026 14:34:51 +0100 Subject: [PATCH] Tests: Add TSan and failing multi-threaded data race test (#7913) PR #7902 uncovered a pre-existing bug in multi-threaded scheduling, where we can end up with an un-ordered R-W hazard in the MTask graph, resulting in non-deterministic runtime behaviour. This is extremely hard to actually trigger on a small example, so using ThreadSanitizer to flag it, which can identify the race reliably. In this patch: - Add configure and `verilator --get-supported TSAN` to check if the configured compiler supports ThreadSanitizer - Add a --tsan option to the test driver.py which builds the test with thread sanitizer (similar idea to --gdbsim). - Add a tests.enable_tsan() method to allow turning on TSan in the test Python file. - Add a suppressions file that waives TSan errors in the runtime library - Finally add `t_sched_hybrid_hazard` that demonstrates the data race triggered after #7902. This is currently expected failing, fix later. With the suppression, there are 17 vltmt tests failing due races in the generated code. (Using `driver.py --vltmt --tsan --quiet -j0`) --- configure.ac | 5 +++ docs/guide/exe_verilator.rst | 2 +- src/V3Options.cpp | 11 +++++++ src/V3Options.h | 7 +++-- src/config_package.h.in | 3 ++ test_regress/driver.py | 37 +++++++++++++++++++++++ test_regress/t/t_flag_supported.py | 6 ++++ test_regress/t/t_sched_hybrid_hazard.py | 22 ++++++++++++++ test_regress/t/t_sched_hybrid_hazard.v | 32 ++++++++++++++++++++ test_regress/t/t_wrapper_clone.py | 3 ++ test_regress/t/trace_lib_as_top_common.py | 3 ++ test_regress/tsan.supp | 33 ++++++++++++++++++++ 12 files changed, 160 insertions(+), 4 deletions(-) create mode 100755 test_regress/t/t_sched_hybrid_hazard.py create mode 100644 test_regress/t/t_sched_hybrid_hazard.v create mode 100644 test_regress/tsan.supp diff --git a/configure.ac b/configure.ac index 893bf91d0..cc37e837b 100644 --- a/configure.ac +++ b/configure.ac @@ -507,6 +507,11 @@ _MY_CXX_CHECK_CORO_SET(CFG_CXXFLAGS_COROUTINES,-std=gnu++20) AC_SUBST(CFG_CXXFLAGS_COROUTINES) AC_SUBST(HAVE_COROUTINES) +# Check if the C++ compiler supports ThreadSanitizer +_MY_CXX_CHECK_IFELSE(-fsanitize=thread, + [AC_DEFINE([HAVE_TSAN],[1],[Defined if ThreadSanitizer is supported by $CXX])]) +AC_SUBST(HAVE_TSAN) + # Flags for compiling Verilator internals including parser always if test "$CFG_WITH_DEV_ASAN" = "yes"; then _MY_CXX_CHECK_IFELSE(-fsanitize=address -DVL_ASAN, diff --git a/docs/guide/exe_verilator.rst b/docs/guide/exe_verilator.rst index 807216565..179fca7dd 100644 --- a/docs/guide/exe_verilator.rst +++ b/docs/guide/exe_verilator.rst @@ -936,7 +936,7 @@ Summary: :file:`*.mk` files. Feature may be one of the following: COROUTINES, DEV_ASAN, DEV_GCOV, - SYSTEMC. + SYSTEMC, TSAN. .. option:: --getenv diff --git a/src/V3Options.cpp b/src/V3Options.cpp index f9502e00c..102906673 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -887,6 +887,8 @@ string V3Options::getSupported(const string& var) { // If update below, also update V3Options::showVersion() if (var == "COROUTINES" && coroutineSupport()) { return "1"; + } else if (var == "TSAN" && tsanSupport()) { + return "1"; } else if (var == "DEV_ASAN" && devAsan()) { return "1"; } else if (var == "DEV_GCOV" && devGcov()) { @@ -936,6 +938,14 @@ bool V3Options::devGcov() { #endif } +bool V3Options::tsanSupport() { +#ifdef HAVE_TSAN + return true; +#else + return false; +#endif +} + //###################################################################### // V3 Options notification methods @@ -2285,6 +2295,7 @@ void V3Options::showVersion(bool verbose) { cout << "Supported features (compiled-in or forced by environment):\n"; cout << " COROUTINES = " << getSupported("COROUTINES") << "\n"; cout << " SYSTEMC = " << getSupported("SYSTEMC") << "\n"; + cout << " TSAN = " << getSupported("TSAN") << "\n"; } //====================================================================== diff --git a/src/V3Options.h b/src/V3Options.h index 182379fe9..dfe8087ae 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -830,9 +830,10 @@ public: static string getSupported(const string& var); static bool systemCSystemWide(); static bool systemCFound(); // SystemC installed, or environment points to it - static bool coroutineSupport(); // Compiler supports coroutines - static bool devAsan(); // Compiler built with AddressSanitizer - static bool devGcov(); // Compiler built with code coverage for gcov + static bool coroutineSupport(); // Configured C++ compiler supports coroutines + static bool devAsan(); // 'verilator_bin' built with AddressSanitizer + static bool devGcov(); // 'verilator_bin' built with code coverage for gcov + static bool tsanSupport(); // Configured C++ compiler supports ThreadSanitizer // METHODS (file utilities using these options) string fileExists(const string& filename); diff --git a/src/config_package.h.in b/src/config_package.h.in index 22909cbde..150a668fd 100644 --- a/src/config_package.h.in +++ b/src/config_package.h.in @@ -40,6 +40,9 @@ PACKAGE_VERSION_STRING_CHAR // Define if coroutines are supported on this platform #undef HAVE_COROUTINES +// Define if ThreadSanitizer is supported on this platform +#undef HAVE_TSAN + // Define if compiled with tcmalloc #undef HAVE_TCMALLOC diff --git a/test_regress/driver.py b/test_regress/driver.py index 0aaca7c16..720c97a35 100755 --- a/test_regress/driver.py +++ b/test_regress/driver.py @@ -146,6 +146,7 @@ class Capabilities: _cached_have_lldb = None _cached_have_sc = None _cached_have_solver = None + _cached_have_tsan = None _cached_make_version = None @staticproperty @@ -226,6 +227,12 @@ class Capabilities: Capabilities._cached_have_solver = bool('usage' in out.casefold()) return Capabilities._cached_have_solver + @staticproperty + def have_tsan() -> bool: # pylint: disable=no-method-argument + if Capabilities._cached_have_tsan is None: + Capabilities._cached_have_tsan = bool(Capabilities._verilator_get_supported('TSAN')) + return Capabilities._cached_have_tsan + @staticproperty @lru_cache(maxsize=1024) def make_version() -> str: # pylint: disable=no-method-argument @@ -248,6 +255,7 @@ class Capabilities: _ignore = Capabilities.have_lldb _ignore = Capabilities.have_sc _ignore = Capabilities.have_solver + _ignore = Capabilities.have_tsan # Internals @@ -881,6 +889,7 @@ class VlTest: self.verilator_make_gmake = True self.verilator_make_cmake = False self.verilated_debug = Args.verilated_debug + self.tsan = Args.tsan self._status_filename = self.obj_dir + "/V" + self.name + ".status" self.coverage_filename = self.obj_dir + "/coverage.dat" @@ -949,6 +958,14 @@ class VlTest: self._skips = message raise VtSkipException + def enable_tsan(self) -> None: + """Called from tests as: enable_tsan() to run this test with + ThreadSanitizer (as if --tsan was given), or skip the test if + ThreadSanitizer is not available.""" + if not self.have_tsan: + self.skip("No ThreadSanitizer-capable C++ compiler available") + self.tsan = True + def priority(self, level: int) -> None: """Called from tests as: priority() to specify what priority order the test should run at. @@ -1138,6 +1155,9 @@ class VlTest: if re.search(r'-runtime-debug', checkflags): self._uses_asan = True + if self.tsan: + # --runtime-debug adds -fsanitize=address, incompatible with thread + self.skip("ThreadSanitizer not compatible with AddressSanitizer\n") verilator_flags = [*param.get('verilator_flags', "")] if Args.gdb: @@ -1150,6 +1170,8 @@ class VlTest: verilator_flags += ["--trace-vcd"] if Args.gdbsim or Args.rrsim: verilator_flags += ["-CFLAGS -ggdb -LDFLAGS -ggdb"] + if self.tsan: + verilator_flags += ["-CFLAGS -fsanitize=thread -CFLAGS -g -LDFLAGS -fsanitize=thread"] verilator_flags += ["--x-assign unique"] # More likely to be buggy if param['vltmt']: @@ -1370,6 +1392,10 @@ class VlTest: "Test requires CMake; ignore error since not available or version too old\n") return + if param['verilator_make_cmake'] and self.tsan: + self.skip("ThreadSanitizer flags not supported with CMake build\n") + return + if not param['fails'] and param['make_main']: self._make_main(param['timing_loop'], param['main_top_name']) @@ -1520,6 +1546,10 @@ class VlTest: run_env = param['run_env'] if run_env: run_env = run_env + ' ' + if self.tsan: + # Use default suppressions; environment TSAN_OPTIONS may override + run_env = ('TSAN_OPTIONS="suppressions=' + os.environ['TEST_REGRESS'] + + '/tsan.supp $TSAN_OPTIONS" ' + run_env) if param['atsim']: cmd = [ @@ -1814,6 +1844,10 @@ class VlTest: self._have_solver_called = True return Capabilities.have_solver + @property + def have_tsan(self) -> bool: + return Capabilities.have_tsan + @property def make_version(self) -> str: return Capabilities.make_version @@ -2998,6 +3032,9 @@ if __name__ == '__main__': parser.add_argument('--stop', action='store_true', help='stop on the first error') parser.add_argument("--top-filename", help="override the default Verilog file name") parser.add_argument('--trace', action='store_true', help='enable simulator waveform tracing') + parser.add_argument('--tsan', + action='store_true', + help='run Verilated executable with ThreadSanitizer') parser.add_argument('--verbose', action='store_true', help='compile and run test in verbose mode') diff --git a/test_regress/t/t_flag_supported.py b/test_regress/t/t_flag_supported.py index b6d5d616c..7da4712a3 100755 --- a/test_regress/t/t_flag_supported.py +++ b/test_regress/t/t_flag_supported.py @@ -23,6 +23,12 @@ if test.have_sc: expect_filename="t/t_flag_supported_1.out", verilator_run=True) +if test.have_tsan: + test.run(cmd=[os.environ["VERILATOR_ROOT"] + "/bin/verilator --get-supported TSAN"], + logfile=test.obj_dir + "/vlt_tsan.log", + expect_filename="t/t_flag_supported_1.out", + verilator_run=True) + test.run(cmd=[os.environ["VERILATOR_ROOT"] + "/bin/verilator --get-supported DOES_NOT_EXIST"], logfile=test.obj_dir + "/vlt_does_not_exist.log", expect_filename="t/t_flag_supported_empty.out", diff --git a/test_regress/t/t_sched_hybrid_hazard.py b/test_regress/t/t_sched_hybrid_hazard.py new file mode 100755 index 000000000..6b088e9bd --- /dev/null +++ b/test_regress/t/t_sched_hybrid_hazard.py @@ -0,0 +1,22 @@ +#!/usr/bin/env python3 +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of either the GNU Lesser General Public License Version 3 +# or the Perl Artistic License Version 2.0. +# SPDX-FileCopyrightText: 2026 Wilson Snyder +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +import vltest_bootstrap + +test.scenarios('vltmt') + +# Very hard to reliably trigger run-time race on small design, +# use ThreadSanitizer to test +test.enable_tsan() + +test.compile(verilator_flags2=['--binary', '-fno-dfg', '--no-threads-coarsen'], threads=2) + +test.execute(fails='any') # Now failing, fix pending + +test.passes() diff --git a/test_regress/t/t_sched_hybrid_hazard.v b/test_regress/t/t_sched_hybrid_hazard.v new file mode 100644 index 000000000..19d53313c --- /dev/null +++ b/test_regress/t/t_sched_hybrid_hazard.v @@ -0,0 +1,32 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain. +// SPDX-FileCopyrightText: 2026 Wilson Snyder +// SPDX-License-Identifier: CC0-1.0 + +// Multi-threaded scheduling hazard on combinational loop variables. + +module t; + logic clk = 0; + always #5 clk = ~clk; + + logic [31:0] cyc = 0; + + // verilator lint_off UNOPTFLAT + logic xxx, yyy; + // verilator lint_on UNOPTFLAT + + // verilator lint_off ALWCOMBORDER + always_comb xxx = cyc[0] ? ~yyy : xxx; // xxx will become hybrid sensitivity + always_comb yyy = cyc[0] ? ~xxx : yyy; // yyy will become hybrid sensitivity + // verilator lint_on ALWCOMBORDER + + always @(posedge clk) begin + cyc <= cyc + 1; + if (cyc == 10) begin + $display("xxx^yyy=%x", xxx ^ yyy); + $write("*-* All Finished *-*\n"); + $finish; + end + end +endmodule diff --git a/test_regress/t/t_wrapper_clone.py b/test_regress/t/t_wrapper_clone.py index cb9e8dec6..1bbe52d47 100755 --- a/test_regress/t/t_wrapper_clone.py +++ b/test_regress/t/t_wrapper_clone.py @@ -11,6 +11,9 @@ import vltest_bootstrap test.scenarios('vlt_all') +if test.tsan: + test.skip("ThreadSanitizer does not support fork from a multithreaded process") + test.compile(make_top_shell=False, make_main=False, verilator_flags2=["--exe", test.pli_filename, "-cc"], diff --git a/test_regress/t/trace_lib_as_top_common.py b/test_regress/t/trace_lib_as_top_common.py index 651e6cecd..97508b877 100644 --- a/test_regress/t/trace_lib_as_top_common.py +++ b/test_regress/t/trace_lib_as_top_common.py @@ -17,6 +17,9 @@ def run(test, *, verilator_flags2=()): if platform.system() == "Windows": test.skip("Skipping on Windows: test depends on Unix-style shared-library loading") + if test.tsan: + test.skip("ThreadSanitizer instrumented library cannot be loaded by python") + # All test use the same SV file test.top_filename = "t/t_trace_lib_as_top.v" test.pli_filename = os.path.abspath("t/t_trace_lib_as_top.cpp") diff --git a/test_regress/tsan.supp b/test_regress/tsan.supp new file mode 100644 index 000000000..bcf71d4c6 --- /dev/null +++ b/test_regress/tsan.supp @@ -0,0 +1,33 @@ +# DESCRIPTION: Verilator: ThreadSanitizer default suppressions for test_regress +# +# This program is free software; you can redistribute it and/or modify it +# under the terms of either the GNU Lesser General Public License Version 3 +# or the Perl Artistic License Version 2.0. +# SPDX-FileCopyrightText: 2026 Wilson Snyder +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 +# +# This file is used via TSAN_OPTIONS by driver.py when running a test with +# ThreadSanitizer (--tsan option, or test.enable_tsan()). Suppress known +# races in the runtime library. TODO: most of these should likely be fixed. + +# Racy update of the cached last-used context pointer +race:*Verilated::lastContextp* + +# Model teardown may destroy mtask state while a worker thread is still +# returning from signaling completion of the final mtask +race:*VlMTaskVertex::signalUpstreamDone* + +# The parallel trace buffer handoff between worker threads and the commit +# on the dumping thread is not visible to ThreadSanitizer; this includes +# the trace worker unlocking a mutex locked by the dumping thread +race:*VerilatedTrace* +race:*VerilatedVcd* +mutex:*VerilatedTrace* + +# vl_stop (e.g. on assertion failure) tears the model down via exit +# handlers while worker threads may still be executing the current eval +race:*vl_stop* + +# The --debug-runtime-timeout alarm handler prints via the runtime, which +# allocates; not async-signal-safe. Note 'signal:' matches the handler name. +signal:*alarmHandler*