From f5998347f475ffaec71a3ab271866879bf2ccbe0 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 4 Dec 2023 13:00:00 -0500 Subject: [PATCH 01/12] Activate tsan on ubuntu --- .github/workflows/firestore.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index d7dcfa8e15f..c183c11a35b 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -240,8 +240,7 @@ jobs: strategy: matrix: - # TODO(b/260248007): Re-enable this once tsan passes on Linux - os: [macos-12] + os: [macos-12, ubuntu-latest] sanitizer: [asan, tsan] runs-on: ${{ matrix.os }} From 07237e59f90d40a07c91b5be5c411a9d47375da5 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 4 Dec 2023 13:34:20 -0500 Subject: [PATCH 02/12] use clang --- .github/workflows/firestore.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index c183c11a35b..122dd795296 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -97,6 +97,14 @@ jobs: - name: Run check run: scripts/check.sh --test-only + build-clang: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: Setup clang + uses: actions/setup-cpp@v2 + with: + compiler: clang-13 cmake: needs: check From 44a01930707a6e10151bd2b4c3aac1e47f520bed Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 4 Dec 2023 13:40:23 -0500 Subject: [PATCH 03/12] Use clang --- .github/workflows/firestore.yml | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index 122dd795296..b89bb0f3717 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -97,15 +97,6 @@ jobs: - name: Run check run: scripts/check.sh --test-only - build-clang: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v2 - - name: Setup clang - uses: actions/setup-cpp@v2 - with: - compiler: clang-13 - cmake: needs: check # Either a scheduled run from public repo, or a pull request with firestore changes. @@ -118,6 +109,8 @@ jobs: env: MINT_PATH: ${{ github.workspace }}/mint + CC: clang + CXX: clang runs-on: ${{ matrix.os }} steps: @@ -168,6 +161,8 @@ jobs: plist_secret: ${{ secrets.GHASecretsGPGPassphrase1 }} MINT_PATH: ${{ github.workspace }}/mint TARGET_DATABASE_ID: ${{ matrix.databaseId }} + CC: clang + CXX: clang runs-on: ${{ matrix.os }} steps: @@ -255,6 +250,8 @@ jobs: env: SANITIZERS: ${{ matrix.sanitizer }} + CC: clang + CXX: clang steps: - uses: actions/checkout@v3 From d3739e442a023acf5690d775777b090f2860fa90 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 4 Dec 2023 14:07:10 -0500 Subject: [PATCH 04/12] revert to default --- .github/workflows/firestore.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index b89bb0f3717..4d48e42d411 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -109,8 +109,6 @@ jobs: env: MINT_PATH: ${{ github.workspace }}/mint - CC: clang - CXX: clang runs-on: ${{ matrix.os }} steps: @@ -161,8 +159,6 @@ jobs: plist_secret: ${{ secrets.GHASecretsGPGPassphrase1 }} MINT_PATH: ${{ github.workspace }}/mint TARGET_DATABASE_ID: ${{ matrix.databaseId }} - CC: clang - CXX: clang runs-on: ${{ matrix.os }} steps: @@ -250,8 +246,6 @@ jobs: env: SANITIZERS: ${{ matrix.sanitizer }} - CC: clang - CXX: clang steps: - uses: actions/checkout@v3 From f042b737ea4c09da70eecf111cd9501b59c65a80 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 4 Dec 2023 14:21:37 -0500 Subject: [PATCH 05/12] disable memleak --- cmake/sanitizer_options.cmake | 1 + 1 file changed, 1 insertion(+) diff --git a/cmake/sanitizer_options.cmake b/cmake/sanitizer_options.cmake index 09735b0b735..ebd971ea2c5 100644 --- a/cmake/sanitizer_options.cmake +++ b/cmake/sanitizer_options.cmake @@ -33,6 +33,7 @@ endmacro() if(CXX_CLANG OR CXX_GNU) if(WITH_ASAN) + set_environment(ASAN_OPTIONS "detect_leaks=0") add_to_compile_and_link_flags("-fsanitize=address") endif() From fe00388b07f0f444ac2b18485f35aad909319441 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 4 Dec 2023 14:31:42 -0500 Subject: [PATCH 06/12] Fix --- cmake/sanitizer_options.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmake/sanitizer_options.cmake b/cmake/sanitizer_options.cmake index ebd971ea2c5..e5f6151e79e 100644 --- a/cmake/sanitizer_options.cmake +++ b/cmake/sanitizer_options.cmake @@ -33,7 +33,7 @@ endmacro() if(CXX_CLANG OR CXX_GNU) if(WITH_ASAN) - set_environment(ASAN_OPTIONS "detect_leaks=0") + set(ASAN_OPTIONS "detect_leaks=0") add_to_compile_and_link_flags("-fsanitize=address") endif() From 76a31f74265acb10b41d4868b5c09ef0e68626bf Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 4 Dec 2023 14:59:24 -0500 Subject: [PATCH 07/12] disable leaks --- .github/workflows/firestore.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index 4d48e42d411..5e8fe53bb94 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -246,6 +246,7 @@ jobs: env: SANITIZERS: ${{ matrix.sanitizer }} + ASAN_OPTIONS: detect_leaks=0 steps: - uses: actions/checkout@v3 From 63a7ec8d763da9a52d58a441c844cdcec98fb93d Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Tue, 5 Dec 2023 14:45:59 -0500 Subject: [PATCH 08/12] try fix --- Firestore/core/src/util/schedule.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Firestore/core/src/util/schedule.cc b/Firestore/core/src/util/schedule.cc index 2a7a9bb33ba..d2d95c23117 100644 --- a/Firestore/core/src/util/schedule.cc +++ b/Firestore/core/src/util/schedule.cc @@ -16,9 +16,10 @@ #include "Firestore/core/src/util/schedule.h" +#include + #include "Firestore/core/src/util/hard_assert.h" #include "Firestore/core/src/util/task.h" -#include "absl/memory/memory.h" namespace firebase { namespace firestore { @@ -29,7 +30,7 @@ Schedule::~Schedule() { } void Schedule::Clear() { - std::unique_lock lock{mutex_}; + std::lock_guard lock{mutex_}; for (Task* task : scheduled_) { task->Release(); From 1e71333aef11995a09695d77ee4c426abd21237d Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 6 Dec 2023 14:55:43 -0500 Subject: [PATCH 09/12] Call it a day --- .github/workflows/firestore.yml | 6 ++++++ cmake/sanitizer_options.cmake | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index 5e8fe53bb94..7bd90c4eaa9 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -241,6 +241,12 @@ jobs: matrix: os: [macos-12, ubuntu-latest] sanitizer: [asan, tsan] + # Excluding TSAN on ubuntu because of the warnings it generates around schedule.cc. + # This could be due to Apple Clang provide additional support for synchronization + # on Apple platforms, which is what we primarily care about. + exclude: + - os: ubuntu-latest + sanitizer: tsan runs-on: ${{ matrix.os }} diff --git a/cmake/sanitizer_options.cmake b/cmake/sanitizer_options.cmake index e5f6151e79e..09735b0b735 100644 --- a/cmake/sanitizer_options.cmake +++ b/cmake/sanitizer_options.cmake @@ -33,7 +33,6 @@ endmacro() if(CXX_CLANG OR CXX_GNU) if(WITH_ASAN) - set(ASAN_OPTIONS "detect_leaks=0") add_to_compile_and_link_flags("-fsanitize=address") endif() From 870f6c2508baadda183431cf0921edbeb3cb0bf2 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Wed, 6 Dec 2023 15:08:03 -0500 Subject: [PATCH 10/12] fix include --- Firestore/core/src/util/schedule.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/Firestore/core/src/util/schedule.cc b/Firestore/core/src/util/schedule.cc index d2d95c23117..619f492bc62 100644 --- a/Firestore/core/src/util/schedule.cc +++ b/Firestore/core/src/util/schedule.cc @@ -16,8 +16,6 @@ #include "Firestore/core/src/util/schedule.h" -#include - #include "Firestore/core/src/util/hard_assert.h" #include "Firestore/core/src/util/task.h" From 104f1f9cce7f0180fcc09fc02a6ad446b323db8c Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Fri, 8 Dec 2023 13:49:00 -0500 Subject: [PATCH 11/12] split sanitizers to make it easy --- .github/workflows/firestore.yml | 49 +++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index 7bd90c4eaa9..3c52e3399a0 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -230,7 +230,7 @@ jobs: scripts/third_party/travis/retry.sh scripts/build.sh Firestore ${{ runner.os }} cmake - sanitizers: + sanitizers-mac: # Either a scheduled run from public repo, or a pull request with firestore changes. if: | (github.repository == 'Firebase/firebase-ios-sdk' && github.event_name == 'schedule') || @@ -239,7 +239,7 @@ jobs: strategy: matrix: - os: [macos-12, ubuntu-latest] + os: [macos-12] sanitizer: [asan, tsan] # Excluding TSAN on ubuntu because of the warnings it generates around schedule.cc. # This could be due to Apple Clang provide additional support for synchronization @@ -250,6 +250,51 @@ jobs: runs-on: ${{ matrix.os }} + env: + SANITIZERS: ${{ matrix.sanitizer }} + + steps: + - uses: actions/checkout@v3 + + - name: Prepare ccache + uses: actions/cache@v3 + with: + path: ${{ runner.temp }}/ccache + key: ${{ matrix.sanitizer }}-firestore-ccache-${{ runner.os }}-${{ github.sha }} + restore-keys: | + ${{ matrix.sanitizer }}-firestore-ccache-${{ runner.os }}- + + - uses: actions/setup-python@v4 + with: + python-version: '3.7' + + - name: Setup build + run: scripts/install_prereqs.sh Firestore ${{ runner.os }} cmake + + - name: Build and test + run: | + export EXPERIMENTAL_MODE=true + export CCACHE_DIR=${{ runner.temp }}/ccache + scripts/third_party/travis/retry.sh scripts/build.sh Firestore ${{ runner.os }} cmake + + + sanitizers-ubuntu: + # Either a scheduled run from public repo, or a pull request with firestore changes. + if: | + (github.repository == 'Firebase/firebase-ios-sdk' && github.event_name == 'schedule') || + (github.event_name == 'pull_request' && needs.changes.outputs.changed == 'true') + needs: check + + strategy: + matrix: + os: [ubuntu-latest] + # Excluding TSAN on ubuntu because of the warnings it generates around schedule.cc. + # This could be due to Apple Clang provide additional support for synchronization + # on Apple platforms, which is what we primarily care about. + sanitizer: [asan] + + runs-on: ${{ matrix.os }} + env: SANITIZERS: ${{ matrix.sanitizer }} ASAN_OPTIONS: detect_leaks=0 From a7e430fe46ee5fbb89c2c742c0a62eccae5aad14 Mon Sep 17 00:00:00 2001 From: Wu-Hui Date: Mon, 11 Dec 2023 11:24:28 -0500 Subject: [PATCH 12/12] feedback --- .github/workflows/firestore.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.github/workflows/firestore.yml b/.github/workflows/firestore.yml index 3c52e3399a0..a5299a44f86 100644 --- a/.github/workflows/firestore.yml +++ b/.github/workflows/firestore.yml @@ -241,12 +241,6 @@ jobs: matrix: os: [macos-12] sanitizer: [asan, tsan] - # Excluding TSAN on ubuntu because of the warnings it generates around schedule.cc. - # This could be due to Apple Clang provide additional support for synchronization - # on Apple platforms, which is what we primarily care about. - exclude: - - os: ubuntu-latest - sanitizer: tsan runs-on: ${{ matrix.os }}