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

Add address/undefined behavior/thread sanitizer builds to CI #373

Merged
merged 5 commits into from
Dec 13, 2022

Conversation

peddie
Copy link
Contributor

@peddie peddie commented Dec 12, 2022

This PR fixes existing runtime sanitizer failures and then adds CI jobs to run the sanitizers on every PR.

@peddie
Copy link
Contributor Author

peddie commented Dec 12, 2022

This cannot be merged for two reasons:

  • Part of Use swift_add_tool() for unit test suite #374 fixes an error that prevents clang-14 from building
  • The address and thread sanitizers both fail due to Possible heap memory error in group-by code #372
    • I cannot mask the failure at the function level because function attributes do not seem to work when applied to templated functions, and the test case that fails is a TYPED_TEST_P, so it's impractical to even pull out the failing bit into a separate function that I can annotate instead.

@peddie peddie force-pushed the peddie/sanitizer-ci branch 5 times, most recently from b5d84e3 to a817fff Compare December 13, 2022 04:44
@peddie peddie changed the title Add address/undefined behavior sanitizer build to CI Add address/undefined behavior/thread sanitizer builds to CI Dec 13, 2022
@peddie peddie requested a review from akleeman December 13, 2022 04:57
@peddie peddie marked this pull request as draft December 13, 2022 04:58
@peddie peddie marked this pull request as ready for review December 13, 2022 07:03
@peddie peddie force-pushed the peddie/sanitizer-ci branch 8 times, most recently from a3588cb to 42366b0 Compare December 13, 2022 09:54
@peddie
Copy link
Contributor Author

peddie commented Dec 13, 2022

I've addressed the issues I mentioned above; it's ready for review.

Copy link
Collaborator

@akleeman akleeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@@ -262,7 +263,7 @@ TYPED_TEST_P(GroupByTester, test_groupby_modify_combine) {
const auto grouped = group_by(parent, this->test_case.get_grouper());
auto groups = grouped.groups();

const auto first_key = map_keys(groups)[0];
const typename decltype(groups)::key_t first_key = map_keys(groups)[0];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why auto doesn't work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It deduces a reference type which has lifetime issues, triggering the address sanitizer!

Comment on lines 95 to 119
ubuntu-clang14-thread:
runs-on: ubuntu-22.04
steps:

- name: Checkout source
uses: actions/checkout@v2
with:
submodules: recursive
ssh-key: ${{ secrets.SSH_KEY }}

- name: Install clang-14
run: |
sudo apt-get update && \
sudo apt-get install build-essential software-properties-common -y && \
sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y && \
sudo apt-get update && \
sudo apt-get install clang-14 && \
gcc -v
- name: Run build
env:
CC: clang-14
CXX: clang++-14
TESTENV: clang14
run: |
bash ./ci/run_thread_sanitizer_tests.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extreme nit. This and the address undefined leak jobs are relatively similar so we can matricize them. Something like:

Suggested change
ubuntu-clang14-thread:
runs-on: ubuntu-22.04
steps:
- name: Checkout source
uses: actions/checkout@v2
with:
submodules: recursive
ssh-key: ${{ secrets.SSH_KEY }}
- name: Install clang-14
run: |
sudo apt-get update && \
sudo apt-get install build-essential software-properties-common -y && \
sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y && \
sudo apt-get update && \
sudo apt-get install clang-14 && \
gcc -v
- name: Run build
env:
CC: clang-14
CXX: clang++-14
TESTENV: clang14
run: |
bash ./ci/run_thread_sanitizer_tests.sh
ubuntu-clang14-sanitizer-tests:
runs-on: ubuntu-22.04
strategy:
os:
- { name: "address-undefined-leak", script: "./ci/run_address_sanitizer_tests.sh" }
- { name: "thread", script: "./ci/run_thread_sanitizer_tests.sh" }
steps:
- name: Checkout source
uses: actions/checkout@v2
with:
submodules: recursive
ssh-key: ${{ secrets.SSH_KEY }}
- name: Install clang-14
run: |
sudo apt-get update
sudo apt-get install build-essential software-properties-common -y
sudo add-apt-repository ppa:ubuntu-toolchain-r/test -y
sudo apt-get update
sudo apt-get install clang-14
gcc -v
- name: Run (${{ matrix.name }} ) Sanitizer Tests
env:
CC: clang-14
CXX: clang++-14
TESTENV: clang14
run: |
bash "${{ matrix.script }}"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done; let's see if I copied correctly :)

submodules: recursive
ssh-key: ${{ secrets.SSH_KEY }}

- name: Install clang-14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might be able to replace this with a premade action, such as:
https://github.com/marketplace/actions/install-llvm-and-clang

Maybe in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to make a ticket for the future -- for now I think I'll just keep it simple.

@peddie peddie merged commit 46fb87e into master Dec 13, 2022
@peddie peddie deleted the peddie/sanitizer-ci branch December 13, 2022 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants