-
Notifications
You must be signed in to change notification settings - Fork 25
Add sanitizer support using GCC toolchain features #64
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
Changes from all commits
6ff6f2c
5190d10
882406f
85fa194
a15595c
0043ef4
5d0c1e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||
| # ******************************************************************************* | ||||
|
pawelrutkaq marked this conversation as resolved.
|
||||
| # Copyright (c) 2026 Contributors to the Eclipse Foundation | ||||
| # | ||||
| # See the NOTICE file(s) distributed with this work for additional | ||||
| # information regarding copyright ownership. | ||||
| # | ||||
| # This program and the accompanying materials are made available under the | ||||
| # terms of the Apache License Version 2.0 which is available at | ||||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||||
| # | ||||
| # SPDX-License-Identifier: Apache-2.0 | ||||
| # ******************************************************************************* | ||||
|
|
||||
| name: Sanitizers | ||||
|
|
||||
| on: | ||||
| pull_request: | ||||
| types: [opened, reopened, synchronize] | ||||
| merge_group: | ||||
| types: [checks_requested] | ||||
|
|
||||
| permissions: | ||||
| contents: read | ||||
|
|
||||
| jobs: | ||||
| sanitizer-tests: | ||||
| name: Bazel Tests (${{ matrix.sanitizer_config }}) | ||||
| runs-on: ubuntu-latest | ||||
| strategy: | ||||
| fail-fast: false | ||||
| matrix: | ||||
| sanitizer_config: [asan_ubsan_lsan, tsan] | ||||
|
|
||||
| steps: | ||||
| - name: Checkout repository | ||||
| uses: actions/checkout@v4.2.2 | ||||
| with: | ||||
| ref: ${{ github.head_ref || github.event.pull_request.head.ref || github.ref }} | ||||
| repository: ${{ github.event.pull_request.head.repo.full_name || github.repository }} | ||||
|
|
||||
| - name: Setup Bazel with shared caching | ||||
| uses: bazel-contrib/setup-bazel@0.18.0 | ||||
| with: | ||||
| bazelisk-version: 1.26.0 | ||||
| disk-cache: true | ||||
| repository-cache: true | ||||
| bazelisk-cache: true | ||||
| cache-save: ${{ github.event_name == 'push' }} | ||||
|
|
||||
| - name: Run sanitizer tests via Bazel | ||||
| run: | | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if e run this in each PR why we would run UT seperatelly ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To verify the unit tests for the config: Line 67 in 5270e48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this runs exactly same UT, so why do we run both instead this one only.?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping the the host unit test runs and the sanitizer runs has advantages since both catch diferent classes of problems and have different goals. One is the fastest way to verify the normal expectations while the latter is sanitized config ensuring quality goals. if your concern is CI budget and CI workflow optimization, we can discuss that seperately and work on a more balanced setup. My suggestion is to bring this in as a baseline for QM quality goals. Take note that we also have mw/log which is an ASIL-B FFI lib. So work products demand this by default.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the normal run catches any kind of different bugs than sanitizer ones. So sanitizer one is superior and catches normal run issues + sanitizer issues or ? Since we run this on PR , the normal run seems to be useless since you have to wait for sanitizers. So still no one answered my questions: in this setup why we run regular C++ unit tests if we want to run one with sanitizers ;)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, lets see: https://github.com/eclipse-score/logging/tree/main/.github/workflows/ build.yml - build onyly, no tests are run neither runs the same. As already mentioned we should not mix coverage report from a sanitizer run!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||||
| set -euo pipefail | ||||
| echo "Running: bazel test --config=${{ matrix.sanitizer_config }} //score/..." | ||||
| # Note: Scoped to C/C++ targets only. Rust targets require Rust-specific | ||||
| # sanitizer handling and are excluded via tag filters. | ||||
| bazel test \ | ||||
| --config=${{ matrix.sanitizer_config }} \ | ||||
| //score/... \ | ||||
| --build_tag_filters=-rust \ | ||||
| --test_tag_filters=-rust \ | ||||
| --verbose_failures | ||||
|
|
||||
| - name: Upload Bazel test logs (always) | ||||
| if: always() | ||||
| uses: actions/upload-artifact@v6 | ||||
| with: | ||||
| name: bazel-testlogs-${{ matrix.sanitizer_config }} | ||||
| path: bazel-testlogs/**/test.log | ||||
| if-no-files-found: warn | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,12 @@ bazel_dep(name = "score_platform", version = "0.5.5", dev_dependency = True) | |
| # Toolchains and extensions | ||
| bazel_dep(name = "score_bazel_cpp_toolchains", version = "0.5.1", dev_dependency = True) | ||
| bazel_dep(name = "score_toolchains_rust", version = "0.8.0", dev_dependency = True) | ||
| bazel_dep(name = "score_cpp_policies", version = "0.0.0", dev_dependency = True) | ||
| git_override( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is done to verify adoption of score_cpp_policies in logging repo. This should be replaced with correct bazel_dep version and not point to a local branch.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nradakovic do we know who is responsible for maintaining and make releases for score_cpp_policies? |
||
| module_name = "score_cpp_policies", | ||
| commit = "6348b27", | ||
| remote = "https://github.com/eclipse-score/score_cpp_policies.git", | ||
| ) | ||
|
|
||
| # S-CORE crates | ||
| bazel_dep(name = "score_crates", version = "0.0.6") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2026 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| filegroup( | ||
| name = "repo_suppressions", | ||
| srcs = [ | ||
| "asan.supp", | ||
| "lsan.supp", | ||
| "tsan.supp", | ||
| "ubsan.supp", | ||
| ], | ||
| visibility = ["//visibility:public"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2026 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| # Suppressions for the AddressSanitizer | ||
| # See: https://clang.llvm.org/docs/AddressSanitizer.html#issue-suppression | ||
| # Every suppression requires a justification. | ||
| # Suppressions that share the same justification may be organized in a single block. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2026 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| # Suppressions for the LeakSanitizer | ||
| # See: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer#suppressions | ||
| # Every suppression requires a justification. | ||
| # Suppressions that share the same justification may be organized in a single block. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2026 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| # Suppressions for the ThreadSanitizer | ||
| # See: https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions | ||
| # Every suppression requires a justification. | ||
| # Suppressions that share the same justification may be organized in a single block. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # ******************************************************************************* | ||
| # Copyright (c) 2026 Contributors to the Eclipse Foundation | ||
| # | ||
| # See the NOTICE file(s) distributed with this work for additional | ||
| # information regarding copyright ownership. | ||
| # | ||
| # This program and the accompanying materials are made available under the | ||
| # terms of the Apache License Version 2.0 which is available at | ||
| # https://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* | ||
|
|
||
| # Suppressions for the UndefinedBehaviorSanitizer | ||
| # See: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#runtime-suppressions | ||
| # Every suppression requires a justification. | ||
| # Suppressions that share the same justification may be organized in a single block. |
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.
Workflow approvals does not seem to run the coverage job!