Fix sanitizer issues in unit tests

Signed-off-by: Aleksandr Ivanov <[email protected]>

.github/workflows/sanitizer-debug.yaml

@@ -0,0 +1,130 @@
+name: Debug sanitizers
+
+on:
+  push:
+    branches:
+      - main
+      - fix-sanitizer-issues
+  pull_request:
+    branches:
+      - main
+
+jobs:
+  build_dependencies:
+    name: Build deps [ubuntu]
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Get dependencies hash
+        id: get-hash
+        run: echo "deps_hash=`cat docker/build_deps.sh | shasum`" >> $GITHUB_OUTPUT
+      - name: Cache lookup
+        uses: actions/cache/restore@v4
+        id: cache-lookup
+        with:
+          path: deps
+          key: deps-${{ steps.get-hash.outputs.deps_hash }}
+          lookup-only: true
+      - name: Set up dependencies
+        if: steps.cache-lookup.outputs.cache-hit != 'true'
+        run: |
+          sudo apt-get update
+          sudo apt-get install -qy build-essential \
+            gdb \
+            curl \
+            python3.10 \
+            python3-pip \
+            cmake \
+            ninja-build \
+            pkg-config \
+            bison \
+            libfl-dev \
+            libbenchmark-dev \
+            libgmock-dev \
+            libz-dev
+      - name: Fetch & Build non packaged dependencies
+        if: steps.cache-lookup.outputs.cache-hit != 'true'
+        run: |
+          mkdir -p deps
+          cd deps
+          ../docker/build_deps.sh
+      - name: Cache save
+        if: steps.cache-lookup.outputs.cache-hit != 'true'
+        uses: actions/cache/save@v4
+        with:
+          path: deps
+          key: deps-${{ steps.get-hash.outputs.deps_hash }}
+
+  unit_tests_cxx:
+    name: UT [c++]
+    runs-on: ubuntu-latest
+    needs: build_dependencies
+    steps:
+      - uses: actions/checkout@v4
+      - name: Get dependencies hash
+        id: get-hash
+        run: echo "deps_hash=`cat docker/build_deps.sh | shasum`" >> $GITHUB_OUTPUT
+      - uses: actions/cache/restore@v4
+        with:
+          path: deps
+          key: deps-${{ steps.get-hash.outputs.deps_hash }}
+      - name: Set up dependencies
+        run: |
+          sudo apt-get update
+          sudo apt-get install -qy build-essential \
+            gdb \
+            curl \
+            python3.10 \
+            python3-pip \
+            cmake \
+            ninja-build \
+            pkg-config \
+            bison \
+            libfl-dev \
+            libbenchmark-dev \
+            libgmock-dev \
+            libz-dev
+      - name: Install cached non packaged dependencies
+        working-directory: deps
+        run: ../docker/build_deps.sh
+      - name: Build BlazingMQ UTs
+        env:
+          PKG_CONFIG_PATH: /usr/lib/x86_64-linux-gnu/pkgconfig:/opt/bb/lib64/pkgconfig
+        run: |
+          cmake -S . -B build/blazingmq -G Ninja \
+            -DCMAKE_TOOLCHAIN_FILE=${{ github.workspace }}/deps/srcs/bde-tools/BdeBuildSystem/toolchains/linux/gcc-default.cmake \
+            -DCMAKE_BUILD_TYPE=Debug \
+            -DBDE_BUILD_TARGET_SAFE=ON \
+            -DBDE_BUILD_TARGET_64=ON \
+            -DBDE_BUILD_TARGET_CPP17=ON \
+            -DCMAKE_PREFIX_PATH=${{ github.workspace }}/deps/srcs/bde-tools/BdeBuildSystem \
+            -DCMAKE_INSTALL_LIBDIR=lib64
+          cmake --build build/blazingmq --parallel 8 --target all.t
+      - name: Run C++ Unit Tests
+        run: |
+          cd ${{ github.workspace }}/build/blazingmq
+          ctest -E mwcsys_executil.t --output-on-failure
+
+  run_asan:
+    needs: build_dependencies
+    uses: ./.github/workflows/sanitizer-check.yaml
+    with:
+      sanitizer-name: 'asan'
+
+  run_msan:
+    needs: build_dependencies
+    uses: ./.github/workflows/sanitizer-check.yaml
+    with:
+      sanitizer-name: 'msan'
+
+  run_tsan:
+    needs: build_dependencies
+    uses: ./.github/workflows/sanitizer-check.yaml
+    with:
+      sanitizer-name: 'tsan'
+
+  run_ubsan:
+    needs: build_dependencies
+    uses: ./.github/workflows/sanitizer-check.yaml
+    with:
+      sanitizer-name: 'ubsan'

etc/tsansup.txt

@@ -15,7 +15,7 @@ race:BloombergLP::bcema_Pool<*>::allocate
 
 # Not sure what the problem is here, but tsan can't show the other stack, so
 # there's nothing to look into
-race:__tsan_atomic32_fetch_add
+#race:__tsan_atomic32_fetch_add
 
 # Don't warn about using cout from multiple threads
 race:std::basic_ostream<char, *>& bsl::operator<< <*>(std::basic_ostream<*>&, bsl::basic_string<*> const&)
@@ -27,8 +27,8 @@ race:BloombergLP::ball::LoggerManager::isInitialized()
 # Suppress TSan report in a routine used in bmqimp::Brokersession test driver.
 # It is a benign race in the test driver, but should be looked into at some
 # point.
-race:TestSession::waitForChannelClose
-race:TestSession::arriveAtStepWithCfgs
+#race:TestSession::waitForChannelClose
+#race:TestSession::arriveAtStepWithCfgs
 
 # Since we use mqbmock::Dispatcher in unit tests, this method does not get
 # enqueued correctly back to cluster-dispatcher thread, causing potential

src/groups/bmq/bmqimp/bmqimp_brokersession.t.cpp

@@ -2196,12 +2196,12 @@ bool TestSession::waitForChannelClose(const bsls::TimeInterval& timeout)
     // Wait for the close to be called on the base channel
     const bsls::TimeInterval expireAfter =
         bsls::SystemTime::nowRealtimeClock() + timeout;
-    while (d_testChannel.closeCalls().empty() &&
+    while (d_testChannel.closeCallsEmpty() &&
            bsls::SystemTime::nowRealtimeClock() < expireAfter) {
         bslmt::ThreadUtil::microSleep(k_TIME_SOURCE_STEP.totalMicroseconds());
     }
 
-    if (d_testChannel.closeCalls().empty()) {
+    if (d_testChannel.closeCallsEmpty()) {
         return false;  // RETURN
     }
 

src/groups/mwc/mwcio/mwcio_ntcchannelfactory.t.cpp

@@ -98,19 +98,22 @@ static const ChannelWatermarkType::Enum WAT_HIGH =
 static const ChannelWatermarkType::Enum WAT_LOW =
     ChannelWatermarkType::e_LOW_WATERMARK;
 
-#ifdef BSLS_PLATFORM_OS_SOLARIS
-static const bool skipTest = true;
+// Adjust attempt interval depending on the platform
+#if defined(BSLS_PLATFORM_OS_SOLARIS) || defined(BSLS_PLATFORM_OS_AIX)
+static const int k_ATTEMPT_INTERVAL_MS = 300;
 #elif defined(                                                                \
     __has_feature)  // Clang-supported method for checking sanitizers.
-static const bool skipTest = __has_feature(memory_sanitizer) ||
-                             __has_feature(thread_sanitizer) ||
-                             __has_feature(undefined_behavior_sanitizer);
+static const int k_ATTEMPT_INTERVAL_MS =
+    (__has_feature(memory_sanitizer) || __has_feature(thread_sanitizer) ||
+     __has_feature(undefined_behavior_sanitizer))
+        ? 300
+        : 1;
 #elif defined(__SANITIZE_MEMORY__) || defined(__SANITIZE_THREAD__) ||         \
     defined(__SANITIZE_UNDEFINED__)
 // GCC-supported macros for checking MSAN, TSAN and UBSAN.
-static const bool skipTest = true;
+static const int k_ATTEMPT_INTERVAL_MS = 300;
 #else
-static const bool skipTest = false;  // Default to running the test.
+static const int k_ATTEMPT_INTERVAL_MS = 1;
 #endif
 
 // ========================
@@ -658,10 +661,9 @@ void Tester::connect(int                          line,
 
     ConnectOptions reqOptions(options, s_allocator_p);
 
-    reqOptions.setAttemptInterval(
-        bsls::TimeInterval(0, 10 * bdlt::TimeUnitRatio::k_NS_PER_MS));
-
-    reqOptions.setAttemptInterval(bsls::TimeInterval(0, 1));
+    reqOptions.setAttemptInterval(bsls::TimeInterval(
+        0,
+        k_ATTEMPT_INTERVAL_MS * bdlt::TimeUnitRatio::k_NS_PER_MS));
 
     HandleMap::iterator serverIter = d_handleMap.find(endpointOrServer);
     if (serverIter != d_handleMap.end()) {
@@ -1055,22 +1057,6 @@ static void test6_preCreationCbTest()
 {
     mwctst::TestHelper::printTestName("Pre Creation Cb Test");
 
-    if (skipTest) {
-        // This test has been disabled for Solaris/MSan/TSan/UBSan build. This
-        // test relies on the timings of certain callbacks being fired before
-        // or after certain operations. Normally this timing is always
-        // observed, but in msan/tsan/ubsan enabled build, the timing gets
-        // changed, leading to test failure. Of course, the right fix is to
-        // not rely on these timings, which can be worked on if the test starts
-        // failing in non-instrumented builds. Additionally, we could try to
-        // enable this test in MSan/TSan/UBSan build once all MSan/TSan/UBSan
-        // reports have been fixed to see if that helps (see `msansup.txt`,
-        // `tsansup.txt` and `ubsansup.txt`).
-        bsl::cout << "Test skipped (running on Solaris or under sanitizer)"
-                  << bsl::endl;
-        return;  // RETURN
-    }
-
     Tester t(s_allocator_p);
 
     // Concern 'a'
@@ -1106,22 +1092,6 @@ static void test5_visitChannelsTest()
 {
     mwctst::TestHelper::printTestName("Cancel Handle Test");
 
-    if (skipTest) {
-        // This test has been disabled for Solaris/MSan/TSan/UBSan build. This
-        // test relies on the timings of certain callbacks being fired before
-        // or after certain operations. Normally this timing is always
-        // observed, but in msan/tsan/ubsan enabled build, the timing gets
-        // changed, leading to test failure. Of course, the right fix is to
-        // not rely on these timings, which can be worked on if the test starts
-        // failing in non-instrumented builds. Additionally, we could try to
-        // enable this test in MSan/TSan/UBSan build once all MSan/TSan/UBSan
-        // reports have been fixed to see if that helps (see `msansup.txt`,
-        // `tsansup.txt` and `ubsansup.txt`).
-        bsl::cout << "Test skipped (running on Solaris or under sanitizer)"
-                  << bsl::endl;
-        return;  // RETURN
-    }
-
     Tester t(s_allocator_p);
 
     // Concerns 'a'
@@ -1162,22 +1132,6 @@ static void test4_cancelHandleTest()
 {
     mwctst::TestHelper::printTestName("Cancel Handle Test");
 
-    if (skipTest) {
-        // This test has been disabled for Solaris/MSan/TSan/UBSan build. This
-        // test relies on the timings of certain callbacks being fired before
-        // or after certain operations. Normally this timing is always
-        // observed, but in msan/tsan/ubsan enabled build, the timing gets
-        // changed, leading to test failure. Of course, the right fix is to
-        // not rely on these timings, which can be worked on if the test starts
-        // failing in non-instrumented builds. Additionally, we could try to
-        // enable this test in MSan/TSan/UBSan build once all MSan/TSan/UBSan
-        // reports have been fixed to see if that helps (see `msansup.txt`,
-        // `tsansup.txt` and `ubsansup.txt`).
-        bsl::cout << "Test skipped (running on Solaris or under sanitizer)"
-                  << bsl::endl;
-        return;  // RETURN
-    }
-
     Tester t(s_allocator_p);
 
     // Concerns 'a'
@@ -1232,22 +1186,6 @@ static void test3_watermarkTest()
 {
     mwctst::TestHelper::printTestName("Watermark Test");
 
-    if (skipTest) {
-        // This test has been disabled for Solaris/MSan/TSan/UBSan build. This
-        // test relies on the timings of certain callbacks being fired before
-        // or after certain operations. Normally this timing is always
-        // observed, but in msan/tsan/ubsan enabled build, the timing gets
-        // changed, leading to test failure. Of course, the right fix is to
-        // not rely on these timings, which can be worked on if the test starts
-        // failing in non-instrumented builds. Additionally, we could try to
-        // enable this test in MSan/TSan/UBSan build once all MSan/TSan/UBSan
-        // reports have been fixed to see if that helps (see `msansup.txt`,
-        // `tsansup.txt` and `ubsansup.txt`).
-        bsl::cout << "Test skipped (running on Solaris or under sanitizer)"
-                  << bsl::endl;
-        return;  // RETURN
-    }
-
     Tester t(s_allocator_p);
 
     // Concern 'a'
@@ -1337,22 +1275,6 @@ static void test1_breathingTest()
 {
     mwctst::TestHelper::printTestName("Breathing Test");
 
-    if (skipTest) {
-        // This test has been disabled for Solaris/MSan/TSan/UBSan build. This
-        // test relies on the timings of certain callbacks being fired before
-        // or after certain operations. Normally this timing is always
-        // observed, but in msan/tsan/ubsan enabled build, the timing gets
-        // changed, leading to test failure. Of course, the right fix is to
-        // not rely on these timings, which can be worked on if the test starts
-        // failing in non-instrumented builds. Additionally, we could try to
-        // enable this test in MSan/TSan/UBSan build once all MSan/TSan/UBSan
-        // reports have been fixed to see if that helps (see `msansup.txt`,
-        // `tsansup.txt` and `ubsansup.txt`).
-        bsl::cout << "Test skipped (running on Solaris or under sanitizer)"
-                  << bsl::endl;
-        return;  // RETURN
-    }
-
     Tester t(s_allocator_p);
     t.init(L_);
 

src/groups/mwc/mwcio/mwcio_testchannel.cpp

@@ -272,6 +272,12 @@ TestChannel::CloseCall TestChannel::popCloseCall()
     return result;
 }
 
+bool TestChannel::closeCallsEmpty()
+{
+    bslmt::LockGuard<bslmt::Mutex> guard(&d_mutex);  // LOCK
+    return d_closeCalls.empty();
+}
+
 TestChannel::CloseSignaler& TestChannel::closeSignaler()
 {
     return d_closeSignaler;

src/groups/mwc/mwcio/mwcio_testchannel.h

@@ -248,6 +248,8 @@ class TestChannel : public Channel {
     /// Pops a close-call from those written to the channel (FIFO ordering).
     CloseCall popCloseCall();
 
+    bool      closeCallsEmpty();
+
     CloseSignaler& closeSignaler();
 
     /// Return a reference providing modifiable access to the corresponding

src/groups/mwc/mwcma/mwcma_countingallocator.t.cpp

@@ -191,23 +191,24 @@ static void test3_deallocate()
     mwctst::TestHelper::printTestName("DEALLOCATE");
 
     // This test twice-deallocates the same block of memory, to verify that
-    // such an operation fails. If we're running under MemorySanitizer or
-    // AddressSanitizer, we must skip this test to avoid detecting the issue
-    // and aborting.
+    // such an operation fails. If we're running under MemorySanitizer,
+    // AddressSanitizer or ThreadSanitizer, we must skip this test to avoid
+    // detecting the issue and aborting.
     //
     // Under MSan, we would instead try to explicitly "unpoison" the memory,
     // but CountingAllocator keeps a hidden "header" block, which we have no
     // good way of accessing to unpoison.
     //
     // Under ASan, we might be able to use the `no_sanitize` attribute, but
-    // GCC doesn't support it before versio 8.0 - so for now, better just to
+    // GCC doesn't support it before version 8.0 - so for now, better just to
     // skip the testcase.
 #if defined(__has_feature)  // Clang-supported method for checking sanitizers.
     const bool skipTestForSanitizers = __has_feature(address_sanitizer) ||
                                        __has_feature(memory_sanitizer) ||
                                        __has_feature(thread_sanitizer);
-#elif defined(__SANITIZE_ADDRESS__) || defined(__SANITIZE_THREAD__)
-    // GCC-supported macros for checking ASAN and TSAN.
+#elif defined(__SANITIZE_ADDRESS__) || defined(__SANITIZE_MEMORY__) ||        \
+    defined(__SANITIZE_THREAD__)
+    // GCC-supported macros for checking ASAN, MSAN and TSAN.
     const bool skipTestForSanitizers = true;
 #else
     const bool skipTestForSanitizers = false;  // Default to running the test.