-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding support for sanitised builds #182
Merged
Merged
Changes from 48 commits
Commits
Show all changes
50 commits
Select commit
Hold shift + click to select a range
285d89b
adding sanitiser flags and workflow file
csegarragonz 949cb34
adding files to diff against
csegarragonz 1e98852
cleaning up workflow file
csegarragonz aa45f75
run only on workflow dispatch
csegarragonz 7e9a4f2
properly use caching
csegarragonz 1c8cc22
setting flag as default to false
csegarragonz 51b7fac
cache the right folder
csegarragonz 43b71d4
Fix ASAN_OPTIONS typo
eigenraven 64bcf5e
Try using cachev2 instead of artifact for conan dependencies
eigenraven b95e2b6
Use a local conan cache folder when using the cli, just like in faasm
eigenraven a5b8c51
UBSan fix: Make sure created int pointer are 4-aligned
eigenraven 815c4ec
ASan,TSan: Latch destruction race-condition fix
eigenraven f5e077b
ASan: MPI World test buffer too small
eigenraven be1b08d
ASan: fix mismatched new[]-delete
eigenraven 45bf978
ASan: out-of-bound read, this was a simple copy-paste error
eigenraven 11c3a77
ASan: Copy temporary strings in RapidJSON
eigenraven d75741c
fix #183: Use unique_ptr for all redis reply objects so they get clea…
eigenraven f43fa32
ASan/leak: use unique_ptr for pulled and dirtyMask to free the arrays…
eigenraven bbc1db2
Change the BYTES macro to a template to catch potential type errors
eigenraven 04bdc9b
Replace all other uses of new/delete with unique_ptr/vector.
eigenraven cc5af1d
Merge remote-tracking branch 'upstream/master' into sanitisers
eigenraven 5b855f5
Add a TSan ignore list
eigenraven 0a921ad
TSan: Fix data race in Scheduler testing code
eigenraven 487791e
Ignore zeromq races
eigenraven aef1cb3
TSan: fix shared_ptr data races (in a shared_ptr the counter is atomi…
eigenraven 680e55d
Ignore config change data races, as they could only affect tests
eigenraven 37f1eac
TSan: Missing lock on scheduler reset
eigenraven ea910c6
Ignore catch2 bad signal handler errors in tsan
eigenraven 745c8d6
TSan: Fix racy executor cleanup/deadlock
eigenraven 72b9b3a
Missing locks in SnapshotClient mock code
eigenraven 935dbe5
MockMode needs to be atomic to work across threads without a mutex
eigenraven feba949
Missing mutexes in FunctionCallClient mock code
eigenraven ad6265e
More missing locks on reads in the scheduler
eigenraven e87360c
Missing locks on *ThisHostResources in Scheduler
eigenraven 0d7d73b
Skip mpi::allGather as I don't know what this exactly does
eigenraven 54ece01
Catch2 REQUIRE is not thread-safe, use assert
eigenraven 12b3d3b
TSan snapshot fixes
eigenraven 0d896a5
Run TSan with maximum history size so it doesn't lose stack traces
eigenraven 7e2e81f
Missing shared lock in KV State
eigenraven 7e0a55a
Disable two tests that break in TSan builds
eigenraven a6ae467
Stop gid generation from tripping TSan using relaxed atomics
eigenraven f7436b5
Avoid racy destruction on FlagWaiter just like the Latch
eigenraven d77f4ae
Missing lock on a queue push
eigenraven e8ac90c
Fix formatting
eigenraven d1ac721
Address review comments
eigenraven 279c9a8
Remove mode checking from python as it's already done by cmake
eigenraven eb58bba
Fix gha cache key, add restore-keys to fall back on cache misses and …
eigenraven fd98afe
Another attempt at fixing checksum hashes, this time use bash instead…
eigenraven d0f3ff8
Merge remote-tracking branch 'upstream/master' into sanitisers
eigenraven d5341e8
Show full test and sanitiser logs, it halts on errors so still should…
eigenraven File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,264 @@ | ||
name: Sanitiser Checks | ||
|
||
on: | ||
push: | ||
branches: [master] | ||
pull_request: | ||
branches: [master] | ||
types: [opened, synchronize, reopened, ready_for_review] | ||
|
||
jobs: | ||
conan-cache: | ||
if: github.event.pull_request.draft == false | ||
runs-on: ubuntu-latest | ||
env: | ||
HOST_TYPE: ci | ||
REDIS_QUEUE_HOST: redis | ||
REDIS_STATE_HOST: redis | ||
container: | ||
image: faasm/faabric:0.2.2 | ||
defaults: | ||
run: | ||
working-directory: /code/faabric | ||
services: | ||
redis: | ||
image: redis | ||
steps: | ||
# --- Code update --- | ||
- name: "Fetch ref" | ||
run: git fetch origin ${GITHUB_REF}:ci-branch | ||
- name: "Check out branch" | ||
run: git checkout --force ci-branch | ||
# --- Set-up --- | ||
- name: "Ping redis" | ||
run: redis-cli -h redis ping | ||
# --- Cache based on Conan version and ExternalProjects cmake source | ||
- name: Get Conan version | ||
id: get-conan-version | ||
run: | | ||
echo "::set-output name=conan-version::$(conan --version | tr -d '[:alpha:] ')" | ||
shell: bash | ||
- name: Hash ExternalProjects | ||
id: hash-external | ||
run: | | ||
echo "::set-output name=hash-external::$(sha256sum cmake/ExternalProjects.cmake | cut '-d ' -f 1)" | ||
shell: bash | ||
- uses: actions/cache@v2 | ||
with: | ||
path: '~/.conan' | ||
key: ${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}-${{ steps.hash-external.outputs.hash-external }} | ||
restore-keys: | | ||
${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}- | ||
${{ runner.os }}- | ||
# --- Build dependencies | ||
- name: "Build dependencies to be shared by all sanitiser runs" | ||
run: inv dev.cmake -b Debug | ||
|
||
address-sanitiser: | ||
if: github.event.pull_request.draft == false | ||
needs: [conan-cache] | ||
runs-on: ubuntu-latest | ||
env: | ||
HOST_TYPE: ci | ||
REDIS_QUEUE_HOST: redis | ||
REDIS_STATE_HOST: redis | ||
container: | ||
image: faasm/faabric:0.2.2 | ||
defaults: | ||
run: | ||
working-directory: /code/faabric | ||
services: | ||
redis: | ||
image: redis | ||
steps: | ||
# --- Code update --- | ||
- name: "Fetch ref" | ||
run: git fetch origin ${GITHUB_REF}:ci-branch | ||
- name: "Check out branch" | ||
run: git checkout --force ci-branch | ||
# --- Set-up --- | ||
- name: "Ping redis" | ||
run: redis-cli -h redis ping | ||
# --- Cache based on Conan version and ExternalProjects cmake source | ||
- name: Get Conan version | ||
id: get-conan-version | ||
run: | | ||
echo "::set-output name=conan-version::$(conan --version | tr -d '[:alpha:] ')" | ||
shell: bash | ||
- name: Hash ExternalProjects | ||
id: hash-external | ||
run: | | ||
echo "::set-output name=hash-external::$(sha256sum cmake/ExternalProjects.cmake | cut '-d ' -f 1)" | ||
shell: bash | ||
- uses: actions/cache@v2 | ||
with: | ||
path: '~/.conan' | ||
key: ${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}-${{ steps.hash-external.outputs.hash-external }} | ||
restore-keys: | | ||
${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}- | ||
${{ runner.os }}- | ||
# --- Build with thread sanitising | ||
- name: "Build tests with address sanitising" | ||
run: inv dev.sanitise Address --noclean | ||
- name: "Run tests with address sanitising on and print to stderr to file" | ||
run: ./bin/faabric_tests > /dev/null | ||
working-directory: /build/faabric/static | ||
env: | ||
ASAN_OPTIONS: "verbosity=1:halt_on_error=1" | ||
|
||
thread-sanitiser: | ||
if: github.event.pull_request.draft == false | ||
needs: [conan-cache] | ||
runs-on: ubuntu-latest | ||
env: | ||
HOST_TYPE: ci | ||
REDIS_QUEUE_HOST: redis | ||
REDIS_STATE_HOST: redis | ||
container: | ||
image: faasm/faabric:0.2.2 | ||
defaults: | ||
run: | ||
working-directory: /code/faabric | ||
services: | ||
redis: | ||
image: redis | ||
steps: | ||
# --- Code update --- | ||
- name: "Fetch ref" | ||
run: git fetch origin ${GITHUB_REF}:ci-branch | ||
- name: "Check out branch" | ||
run: git checkout --force ci-branch | ||
# --- Set-up --- | ||
- name: "Ping redis" | ||
run: redis-cli -h redis ping | ||
# --- Cache based on Conan version and ExternalProjects cmake source | ||
- name: Get Conan version | ||
id: get-conan-version | ||
run: | | ||
echo "::set-output name=conan-version::$(conan --version | tr -d '[:alpha:] ')" | ||
shell: bash | ||
- name: Hash ExternalProjects | ||
id: hash-external | ||
run: | | ||
echo "::set-output name=hash-external::$(sha256sum cmake/ExternalProjects.cmake | cut '-d ' -f 1)" | ||
shell: bash | ||
- uses: actions/cache@v2 | ||
with: | ||
path: '~/.conan' | ||
key: ${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}-${{ steps.hash-external.outputs.hash-external }} | ||
restore-keys: | | ||
${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}- | ||
${{ runner.os }}- | ||
# --- Build with thread sanitising | ||
- name: "Build tests with thread sanitising" | ||
run: inv dev.sanitise Thread --noclean | ||
# --- Tests --- | ||
- name: "Run tests with thread sanitising on and print stderr to file" | ||
run: ./bin/faabric_tests > /dev/null | ||
working-directory: /build/faabric/static | ||
env: | ||
TSAN_OPTIONS: "verbosity=1:halt_on_error=1:suppressions=/code/faabric/thread-sanitizer-ignorelist.txt:history_size=7" | ||
|
||
undefined-sanitiser: | ||
if: github.event.pull_request.draft == false | ||
needs: [conan-cache] | ||
runs-on: ubuntu-latest | ||
env: | ||
HOST_TYPE: ci | ||
REDIS_QUEUE_HOST: redis | ||
REDIS_STATE_HOST: redis | ||
container: | ||
image: faasm/faabric:0.2.2 | ||
defaults: | ||
run: | ||
working-directory: /code/faabric | ||
services: | ||
redis: | ||
image: redis | ||
steps: | ||
# --- Code update --- | ||
- name: "Fetch ref" | ||
run: git fetch origin ${GITHUB_REF}:ci-branch | ||
- name: "Check out branch" | ||
run: git checkout --force ci-branch | ||
# --- Set-up --- | ||
- name: "Ping redis" | ||
run: redis-cli -h redis ping | ||
# --- Cache based on Conan version and ExternalProjects cmake source | ||
- name: Get Conan version | ||
id: get-conan-version | ||
run: | | ||
echo "::set-output name=conan-version::$(conan --version | tr -d '[:alpha:] ')" | ||
shell: bash | ||
- name: Hash ExternalProjects | ||
id: hash-external | ||
run: | | ||
echo "::set-output name=hash-external::$(sha256sum cmake/ExternalProjects.cmake | cut '-d ' -f 1)" | ||
shell: bash | ||
- uses: actions/cache@v2 | ||
with: | ||
path: '~/.conan' | ||
key: ${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}-${{ steps.hash-external.outputs.hash-external }} | ||
restore-keys: | | ||
${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}- | ||
${{ runner.os }}- | ||
# --- Build with thread sanitising | ||
- name: "Build tests with undefined sanitising" | ||
run: inv dev.sanitise Undefined --noclean | ||
# --- Tests --- | ||
- name: "Run tests with undefined sanitising on and print stderr to file" | ||
run: ./bin/faabric_tests > /dev/null | ||
working-directory: /build/faabric/static | ||
env: | ||
UBSAN_OPTIONS: "print_stacktrace=1:halt_on_error=1" | ||
|
||
leak-sanitiser: | ||
if: github.event.pull_request.draft == false | ||
needs: [conan-cache] | ||
runs-on: ubuntu-latest | ||
env: | ||
HOST_TYPE: ci | ||
REDIS_QUEUE_HOST: redis | ||
REDIS_STATE_HOST: redis | ||
container: | ||
image: faasm/faabric:0.2.2 | ||
defaults: | ||
run: | ||
working-directory: /code/faabric | ||
services: | ||
redis: | ||
image: redis | ||
steps: | ||
# --- Code update --- | ||
- name: "Fetch ref" | ||
run: git fetch origin ${GITHUB_REF}:ci-branch | ||
- name: "Check out branch" | ||
run: git checkout --force ci-branch | ||
# --- Set-up --- | ||
- name: "Ping redis" | ||
run: redis-cli -h redis ping | ||
# --- Cache based on Conan version and ExternalProjects cmake source | ||
- name: Get Conan version | ||
id: get-conan-version | ||
run: | | ||
echo "::set-output name=conan-version::$(conan --version | tr -d '[:alpha:] ')" | ||
shell: bash | ||
- name: Hash ExternalProjects | ||
id: hash-external | ||
run: | | ||
echo "::set-output name=hash-external::$(sha256sum cmake/ExternalProjects.cmake | cut '-d ' -f 1)" | ||
shell: bash | ||
- uses: actions/cache@v2 | ||
with: | ||
path: '~/.conan' | ||
key: ${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}-${{ steps.hash-external.outputs.hash-external }} | ||
restore-keys: | | ||
${{ runner.os }}-${{ steps.get-conan-version.outputs.conan-version }}- | ||
${{ runner.os }}- | ||
# --- Build with thread sanitising | ||
- name: "Build tests with leak sanitising" | ||
run: inv dev.sanitise Leak --noclean | ||
# --- Tests --- | ||
- name: "Run tests with leak sanitising on and print stderr to file" | ||
run: ./bin/faabric_tests > /dev/null | ||
working-directory: /build/faabric/static |
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
build/ | ||
|
||
conan-cache/ | ||
# Clang | ||
.clangd | ||
compile_commands.json | ||
|
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a job for each different sanitizer? This file is quite bloated as a result, so I'd be interested to know the time difference in running them sequentially in a single job, repeating the call to
dev.sanitise
andfaabric_tests
each time. The GHA docs say each job in a workflow runs in a "fresh instance" of the VM, but I don't know whether this means a completely separate VM on a different host, or if it's actually just cramming lots of VMs together on the same allocated physical resources (in which case it might be counterproductive).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all runs in parallel, and each sanitizer requires a full rebuild of the source (but not dependencies), so this saves about 20 mins of time on tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool, is that in theory or in practice? I'm just surprised that GHA gives out resources that freely. I don't think we should change it, am just curious.