From 509dac0ea2ae435a90b340f3911fd18e4679b17e Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 10 Apr 2024 11:08:58 -0700 Subject: [PATCH 1/4] Add TSAN suppressions for the free-threaded build Add TSAN suppressions for existing races in the free-threaded build. --- .github/workflows/build.yml | 2 + .github/workflows/reusable-tsan.yml | 6 ++- Tools/tsan/suppressions_free_threading.txt | 51 ++++++++++++++++++++++ Tools/tsan/supressions.txt | 3 +- 4 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 Tools/tsan/suppressions_free_threading.txt diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 9e236534ae3770..e1a2a62c60c6de 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -492,6 +492,7 @@ jobs: with: config_hash: ${{ needs.check_source.outputs.config_hash }} options: ./configure --config-cache --with-thread-sanitizer --with-pydebug + suppressions_path: Tools/tsan/supressions.txt build_tsan_free_threading: name: 'Thread sanitizer (free-threading)' @@ -501,6 +502,7 @@ jobs: with: config_hash: ${{ needs.check_source.outputs.config_hash }} options: ./configure --config-cache --disable-gil --with-thread-sanitizer --with-pydebug + suppressions_path: Tools/tsan/suppressions_free_threading.txt # CIFuzz job based on https://google.github.io/oss-fuzz/getting-started/continuous-integration/ cifuzz: diff --git a/.github/workflows/reusable-tsan.yml b/.github/workflows/reusable-tsan.yml index 96a9c1b0cda3c3..8ddb3b3ada32c2 100644 --- a/.github/workflows/reusable-tsan.yml +++ b/.github/workflows/reusable-tsan.yml @@ -7,6 +7,10 @@ on: options: required: true type: string + suppressions_path: + description: 'A repo relative path to the suppressions file' + required: true + type: string jobs: build_tsan_reusable: @@ -30,7 +34,7 @@ jobs: sudo sysctl -w vm.mmap_rnd_bits=28 - name: TSAN Option Setup run: | - echo "TSAN_OPTIONS=suppressions=${GITHUB_WORKSPACE}/Tools/tsan/supressions.txt" >> $GITHUB_ENV + echo "TSAN_OPTIONS=suppressions=${GITHUB_WORKSPACE}/${{ inputs.suppressions_path }}" >> $GITHUB_ENV echo "CC=clang" >> $GITHUB_ENV echo "CXX=clang++" >> $GITHUB_ENV - name: Add ccache to PATH diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt new file mode 100644 index 00000000000000..efdc67e9d27164 --- /dev/null +++ b/Tools/tsan/suppressions_free_threading.txt @@ -0,0 +1,51 @@ +# This file contains suppressions for the free-threaded build. It contains the +# suppressions for the default build and additional suppressions needed only in +# the free-threaded build. +# +# reference: https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions + +## Default build suppresssions + +race:get_allocator_unlocked +race:set_allocator_unlocked +race:mi_heap_visit_pages +race:_mi_heap_delayed_free_partial + +## Free-threaded suppressions + +race:_add_to_weak_set +race:_in_weak_set +race:_Py_IsImmortal +race:_Py_IsOwnedByCurrentThread +race:_PyEval_EvalFrameDefault +race:_PyFunction_SetVersion +race:_PyImport_AcquireLock +race:_PyImport_ReleaseLock +race:_PyInterpreterState_SetNotRunningMain +race:_PyInterpreterState_IsRunningMain +race:_PyObject_GC_IS_SHARED +race:_PyObject_GC_SET_SHARED +race:_PyObject_GC_TRACK +race:_PyType_HasFeature +race:_PyType_Lookup +race:assign_version_tag +race:compare_unicode_unicode +race:delitem_common +race:dictkeys_decref +race:dictkeys_incref +race:dictresize +race:gc_collect_main +race:gc_restore_tid +race:initialize_new_array +race:insertdict +race:lookup_tp_dict +race:PyMember_GetOne +race:PyMember_SetOne +race:new_reference +race:set_contains_key +race:set_inheritable +race:start_the_world +race:tstate_set_detached +race:unicode_hash +race:update_cache +race:update_cache_gil_disabled diff --git a/Tools/tsan/supressions.txt b/Tools/tsan/supressions.txt index 448dfac8005c79..f977fd070c4789 100644 --- a/Tools/tsan/supressions.txt +++ b/Tools/tsan/supressions.txt @@ -1,4 +1,5 @@ -## reference: https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions +# This file contains suppressions for the default (with GIL) build. +# reference: https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions race:get_allocator_unlocked race:set_allocator_unlocked race:mi_heap_visit_pages From 1d44c87996f2468cd0ccb69956e7c7cfe6a40101 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Wed, 10 Apr 2024 16:39:16 -0700 Subject: [PATCH 2/4] Skip prohibitively slow tests --- Lib/test/test_weakref.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index d0e8df4ea82802..6ae4dc48f77bb4 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -101,6 +101,11 @@ def collect(): t.join() +skip_if_tsan_and_gil_disabled = unittest.skipIf( + support.check_sanitizer(thread=True) and support.Py_GIL_DISABLED, + "Test is prohibitively slow with TSAN enabled and the GIL disabled") + + class ReferencesTestCase(TestBase): def test_basic_ref(self): @@ -1877,6 +1882,7 @@ def test_make_weak_keyed_dict_repr(self): self.assertRegex(repr(dict), '') @threading_helper.requires_working_threading() + @skip_if_tsan_and_gil_disabled def test_threaded_weak_valued_setdefault(self): d = weakref.WeakValueDictionary() with collect_in_thread(): @@ -1886,6 +1892,7 @@ def test_threaded_weak_valued_setdefault(self): del x @threading_helper.requires_working_threading() + @skip_if_tsan_and_gil_disabled def test_threaded_weak_valued_pop(self): d = weakref.WeakValueDictionary() with collect_in_thread(): @@ -1895,6 +1902,7 @@ def test_threaded_weak_valued_pop(self): self.assertIsNot(x, None) # we never put None in there! @threading_helper.requires_working_threading() + @skip_if_tsan_and_gil_disabled def test_threaded_weak_valued_consistency(self): # Issue #28427: old keys should not remove new values from # WeakValueDictionary when collecting from another thread. From 8f18671d257c2ed0449bc41cc1c6fd72fb7385cb Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 11 Apr 2024 10:14:00 -0700 Subject: [PATCH 3/4] Categorize mimalloc races --- Tools/tsan/suppressions_free_threading.txt | 4 ++-- Tools/tsan/supressions.txt | 2 -- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index efdc67e9d27164..889b62e59b14a6 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -8,13 +8,12 @@ race:get_allocator_unlocked race:set_allocator_unlocked -race:mi_heap_visit_pages -race:_mi_heap_delayed_free_partial ## Free-threaded suppressions race:_add_to_weak_set race:_in_weak_set +race:_mi_heap_delayed_free_partial race:_Py_IsImmortal race:_Py_IsOwnedByCurrentThread race:_PyEval_EvalFrameDefault @@ -39,6 +38,7 @@ race:gc_restore_tid race:initialize_new_array race:insertdict race:lookup_tp_dict +race:mi_heap_visit_pages race:PyMember_GetOne race:PyMember_SetOne race:new_reference diff --git a/Tools/tsan/supressions.txt b/Tools/tsan/supressions.txt index f977fd070c4789..c778c791eacce8 100644 --- a/Tools/tsan/supressions.txt +++ b/Tools/tsan/supressions.txt @@ -2,5 +2,3 @@ # reference: https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions race:get_allocator_unlocked race:set_allocator_unlocked -race:mi_heap_visit_pages -race:_mi_heap_delayed_free_partial From 1ea81079e1159dce2e7c740a7e8f35cb3cee39aa Mon Sep 17 00:00:00 2001 From: Matt Page Date: Thu, 11 Apr 2024 10:30:54 -0700 Subject: [PATCH 4/4] Reduce iteration count instead of skipping tests --- Lib/test/test_weakref.py | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index 6ae4dc48f77bb4..499ba77fd19542 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -101,11 +101,6 @@ def collect(): t.join() -skip_if_tsan_and_gil_disabled = unittest.skipIf( - support.check_sanitizer(thread=True) and support.Py_GIL_DISABLED, - "Test is prohibitively slow with TSAN enabled and the GIL disabled") - - class ReferencesTestCase(TestBase): def test_basic_ref(self): @@ -1260,6 +1255,12 @@ class MappingTestCase(TestBase): COUNT = 10 + if support.check_sanitizer(thread=True) and support.Py_GIL_DISABLED: + # Reduce iteration count to get acceptable latency + NUM_THREADED_ITERATIONS = 1000 + else: + NUM_THREADED_ITERATIONS = 100000 + def check_len_cycles(self, dict_type, cons): N = 20 items = [RefCycle() for i in range(N)] @@ -1882,33 +1883,30 @@ def test_make_weak_keyed_dict_repr(self): self.assertRegex(repr(dict), '') @threading_helper.requires_working_threading() - @skip_if_tsan_and_gil_disabled def test_threaded_weak_valued_setdefault(self): d = weakref.WeakValueDictionary() with collect_in_thread(): - for i in range(100000): + for i in range(self.NUM_THREADED_ITERATIONS): x = d.setdefault(10, RefCycle()) self.assertIsNot(x, None) # we never put None in there! del x @threading_helper.requires_working_threading() - @skip_if_tsan_and_gil_disabled def test_threaded_weak_valued_pop(self): d = weakref.WeakValueDictionary() with collect_in_thread(): - for i in range(100000): + for i in range(self.NUM_THREADED_ITERATIONS): d[10] = RefCycle() x = d.pop(10, 10) self.assertIsNot(x, None) # we never put None in there! @threading_helper.requires_working_threading() - @skip_if_tsan_and_gil_disabled def test_threaded_weak_valued_consistency(self): # Issue #28427: old keys should not remove new values from # WeakValueDictionary when collecting from another thread. d = weakref.WeakValueDictionary() with collect_in_thread(): - for i in range(200000): + for i in range(2 * self.NUM_THREADED_ITERATIONS): o = RefCycle() d[10] = o # o is still alive, so the dict can't be empty