Skip to content
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 50 commits into from
Nov 30, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
285d89b
adding sanitiser flags and workflow file
csegarragonz Nov 25, 2021
949cb34
adding files to diff against
csegarragonz Nov 25, 2021
1e98852
cleaning up workflow file
csegarragonz Nov 25, 2021
aa45f75
run only on workflow dispatch
csegarragonz Nov 25, 2021
7e9a4f2
properly use caching
csegarragonz Nov 25, 2021
1c8cc22
setting flag as default to false
csegarragonz Nov 25, 2021
51b7fac
cache the right folder
csegarragonz Nov 26, 2021
43b71d4
Fix ASAN_OPTIONS typo
eigenraven Nov 26, 2021
64bcf5e
Try using cachev2 instead of artifact for conan dependencies
eigenraven Nov 26, 2021
b95e2b6
Use a local conan cache folder when using the cli, just like in faasm
eigenraven Nov 26, 2021
a5b8c51
UBSan fix: Make sure created int pointer are 4-aligned
eigenraven Nov 26, 2021
815c4ec
ASan,TSan: Latch destruction race-condition fix
eigenraven Nov 26, 2021
f5e077b
ASan: MPI World test buffer too small
eigenraven Nov 26, 2021
be1b08d
ASan: fix mismatched new[]-delete
eigenraven Nov 26, 2021
45bf978
ASan: out-of-bound read, this was a simple copy-paste error
eigenraven Nov 26, 2021
11c3a77
ASan: Copy temporary strings in RapidJSON
eigenraven Nov 26, 2021
d75741c
fix #183: Use unique_ptr for all redis reply objects so they get clea…
eigenraven Nov 26, 2021
f43fa32
ASan/leak: use unique_ptr for pulled and dirtyMask to free the arrays…
eigenraven Nov 26, 2021
bbc1db2
Change the BYTES macro to a template to catch potential type errors
eigenraven Nov 26, 2021
04bdc9b
Replace all other uses of new/delete with unique_ptr/vector.
eigenraven Nov 26, 2021
cc5af1d
Merge remote-tracking branch 'upstream/master' into sanitisers
eigenraven Nov 26, 2021
5b855f5
Add a TSan ignore list
eigenraven Nov 26, 2021
0a921ad
TSan: Fix data race in Scheduler testing code
eigenraven Nov 26, 2021
487791e
Ignore zeromq races
eigenraven Nov 26, 2021
aef1cb3
TSan: fix shared_ptr data races (in a shared_ptr the counter is atomi…
eigenraven Nov 26, 2021
680e55d
Ignore config change data races, as they could only affect tests
eigenraven Nov 26, 2021
37f1eac
TSan: Missing lock on scheduler reset
eigenraven Nov 26, 2021
ea910c6
Ignore catch2 bad signal handler errors in tsan
eigenraven Nov 26, 2021
745c8d6
TSan: Fix racy executor cleanup/deadlock
eigenraven Nov 26, 2021
72b9b3a
Missing locks in SnapshotClient mock code
eigenraven Nov 26, 2021
935dbe5
MockMode needs to be atomic to work across threads without a mutex
eigenraven Nov 26, 2021
feba949
Missing mutexes in FunctionCallClient mock code
eigenraven Nov 26, 2021
ad6265e
More missing locks on reads in the scheduler
eigenraven Nov 26, 2021
e87360c
Missing locks on *ThisHostResources in Scheduler
eigenraven Nov 26, 2021
0d7d73b
Skip mpi::allGather as I don't know what this exactly does
eigenraven Nov 26, 2021
54ece01
Catch2 REQUIRE is not thread-safe, use assert
eigenraven Nov 26, 2021
12b3d3b
TSan snapshot fixes
eigenraven Nov 26, 2021
0d896a5
Run TSan with maximum history size so it doesn't lose stack traces
eigenraven Nov 26, 2021
7e2e81f
Missing shared lock in KV State
eigenraven Nov 26, 2021
7e0a55a
Disable two tests that break in TSan builds
eigenraven Nov 26, 2021
a6ae467
Stop gid generation from tripping TSan using relaxed atomics
eigenraven Nov 26, 2021
f7436b5
Avoid racy destruction on FlagWaiter just like the Latch
eigenraven Nov 26, 2021
d77f4ae
Missing lock on a queue push
eigenraven Nov 26, 2021
e8ac90c
Fix formatting
eigenraven Nov 26, 2021
d1ac721
Address review comments
eigenraven Nov 29, 2021
279c9a8
Remove mode checking from python as it's already done by cmake
eigenraven Nov 29, 2021
eb58bba
Fix gha cache key, add restore-keys to fall back on cache misses and …
eigenraven Nov 29, 2021
fd98afe
Another attempt at fixing checksum hashes, this time use bash instead…
eigenraven Nov 29, 2021
d0f3ff8
Merge remote-tracking branch 'upstream/master' into sanitisers
eigenraven Nov 30, 2021
d5341e8
Show full test and sanitiser logs, it halts on errors so still should…
eigenraven Nov 30, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 200 additions & 0 deletions .github/workflows/sanitisers.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
name: Sanitiser Checks

# on: workflow_dispatch
eigenraven marked this conversation as resolved.
Show resolved Hide resolved
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
# --- Build dependencies
- name: "Build dependencies to be shared by all sanitiser runs"
run: inv dev.cmake -b Debug
- name: "Upload built CMake dependencies"
uses: actions/upload-artifact@v2
with:
name: cmake-cache
path: /build/faabric
eigenraven marked this conversation as resolved.
Show resolved Hide resolved

address-sanitiser:
Copy link
Collaborator

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 and faabric_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).

Copy link
Collaborator

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

Copy link
Collaborator

@Shillaker Shillaker Nov 30, 2021

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.

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
# --- Build with thread sanitising
- name: "Download CMake dependencies"
uses: actions/download-artifact@v2
with:
name: cmake-cache
path: /build/faabric
- 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:
ADSAN_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
# --- Build with thread sanitising
- name: "Download CMake dependencies"
uses: actions/download-artifact@v2
with:
name: cmake-cache
path: /build/faabric
- 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"

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
# --- Build with thread sanitising
- name: "Download CMake dependencies"
uses: actions/download-artifact@v2
with:
name: cmake-cache
path: /build/faabric
- 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
# --- Build with thread sanitising
- name: "Download CMake dependencies"
uses: actions/download-artifact@v2
with:
name: cmake-cache
path: /build/faabric
- 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
13 changes: 13 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ set(CMAKE_EXE_LINKER_FLAGS "-fuse-ld=lld")
# Compile comamnds for clang tools
set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

# Set-up use of sanitisers
if (FAABRIC_USE_SANITISER STREQUAL "Address")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address")
elseif (FAABRIC_USE_SANITISER STREQUAL "Thread")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=thread")
elseif (FAABRIC_USE_SANITISER STREQUAL "Undefined")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined")
elseif (FAABRIC_USE_SANITISER STREQUAL "Leak")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=leak")
elseif (FAABRIC_USE_SANITISER STREQUAL "Memory")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=memory")
endif()
eigenraven marked this conversation as resolved.
Show resolved Hide resolved

# Global include dir
set(FAABRIC_INCLUDE_DIR ${CMAKE_CURRENT_LIST_DIR}/include)

Expand Down
25 changes: 24 additions & 1 deletion tasks/dev.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@


@task
def cmake(ctx, clean=False, shared=False, build="Debug"):
def cmake(ctx, clean=False, shared=False, build="Debug", sanitise_mode="None"):
"""
Configures the build
"""
Expand All @@ -40,6 +40,7 @@ def cmake(ctx, clean=False, shared=False, build="Debug"):
"-DBUILD_SHARED_LIBS={}".format("ON" if shared else "OFF"),
"-DCMAKE_CXX_COMPILER=/usr/bin/clang++-13",
"-DCMAKE_C_COMPILER=/usr/bin/clang-13",
"-DFAABRIC_USE_SANITISER={}".format(sanitise_mode),
PROJ_ROOT,
]

Expand Down Expand Up @@ -78,3 +79,25 @@ def install(ctx, target, shared=False):
cwd=build_dir,
shell=True,
)


@task
def sanitise(ctx, mode, noclean=False, shared=False):
"""
Build the tests with different sanitisers
"""
mode_list = ["Address", "Thread", "Undefined", "Leak", "Memory"]
if mode not in mode_list:
print("Unrecognised sanitiser mode: {}".format(mode))
print("Sanitisier mode must be one in {}".format(mode_list))
return
eigenraven marked this conversation as resolved.
Show resolved Hide resolved

cmake(
ctx,
clean=(not noclean),
shared=shared,
build="Debug",
sanitise_mode=mode,
)

cc(ctx, "faabric_tests", shared=shared)