Initial sanitizer configurations#1
Conversation
Signed-off-by: rahul.singh <rahul.sa.singh@partner.bmwgroup.com>
Signed-off-by: rahul.singh <rahul.sa.singh@partner.bmwgroup.com>
.github/workflows/tests.yml
Outdated
| jobs: | ||
| test-sanitizers: | ||
| name: Validate sanitizer configs | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
why using latest? this can easily break our workflows in case it gets upgraded
There was a problem hiding this comment.
Fixed to ubuntu-24.04 for stability
tests/sample_test.cpp
Outdated
| EXPECT_EQ(counter.load(), 4000); | ||
| } | ||
|
|
||
| // Test for undefined behavior detection (UBSan) |
There was a problem hiding this comment.
this comment is highly misleading. the below test does not contain any UB hence, how shall it help for verifying UB detection? better adjust the comment
tests/sample_test.cpp
Outdated
| EXPECT_EQ(vec[4], 5); | ||
| } | ||
|
|
||
| // Test that allocates memory (for ASan/LSan validation) |
There was a problem hiding this comment.
how about
| // Test that allocates memory (for ASan/LSan validation) | |
| // Memory allocation test that should pass with all sanitizers |
?
tests/MODULE.bazel
Outdated
| bazel_dep(name = "score_bazel_cpp_toolchains", version = "0.2.2") | ||
| bazel_dep(name = "score_bazel_platforms", version = "0.0.4") |
There was a problem hiding this comment.
Why are these two required here? Where are they used actually?
There was a problem hiding this comment.
I have a follow-up question: why aren’t we using these platforms & toolchains here?
The reference integration relies on these toolchains configurations for release verification. If we intend for others to adopt these workflows, we should validate them against the same toolchains used in reference integration. Otherwise, we introduce an additional risk of failure due to configuration mismatches or incompatibilities.
Of course, the choice of what to test is ultimately yours. However, if the goal is for this to serve as the primary workflow for sanitizer testing across the project, then it should also pass reference integration—i.e., it must be validated with our default toolchain setups.
README.md
Outdated
| - `--config=asan` - AddressSanitizer (memory errors, buffer overflows) | ||
| - `--config=tsan` - ThreadSanitizer (data races, deadlocks) | ||
| - `--config=ubsan` - UndefinedBehaviorSanitizer (undefined behavior) | ||
| - `--config=lsan` - LeakSanitizer (memory leaks) |
There was a problem hiding this comment.
prefer alphabetical sorting instead?
| - `--config=asan` - AddressSanitizer (memory errors, buffer overflows) | |
| - `--config=tsan` - ThreadSanitizer (data races, deadlocks) | |
| - `--config=ubsan` - UndefinedBehaviorSanitizer (undefined behavior) | |
| - `--config=lsan` - LeakSanitizer (memory leaks) | |
| - `--config=asan` - AddressSanitizer (memory errors, buffer overflows) | |
| - `--config=lsan` - LeakSanitizer (memory leaks) | |
| - `--config=tsan` - ThreadSanitizer (data races, deadlocks) | |
| - `--config=ubsan` - UndefinedBehaviorSanitizer (undefined behavior) |
sanitizer/sanitizer.bazelrc
Outdated
There was a problem hiding this comment.
how about preferring plural here? would be more appropriate in my eyes since it makes clear that it will contain multiple sanitizer configs
i.e. sanitizers/sanitizers.bazelrc
sanitizer/sanitizer.bazelrc
Outdated
| # ******************************************************************************* | ||
|
|
||
| # ASan + UBSan + LSan (Combined - recommended for most testing) | ||
| test:asan_ubsan_lsan --compilation_mode=dbg |
There was a problem hiding this comment.
please do not use dbg compliation mode. this would potentially enable code which is not part of the actual production code and hence you would potentially run into different code paths. just omit that everywhere since such compilation flags must come from the toolchain which defines the respective sanitizer features.
also see https://github.com/eclipse-score/communication/blob/main/quality/sanitizer/sanitizer.bazelrc#L20-L30 for further reference which flags we require for sanitizer configs
There was a problem hiding this comment.
Removed compilation_mode=dbg and Done as suggested.
sanitizer/sanitizer.bazelrc
Outdated
| test:tsan --compilation_mode=dbg | ||
| test:tsan --features=tsan | ||
| test:tsan --platform_suffix=tsan |
There was a problem hiding this comment.
here and also for all the other configs:
| test:tsan --compilation_mode=dbg | |
| test:tsan --features=tsan | |
| test:tsan --platform_suffix=tsan | |
| build:tsan --features=tsan | |
| build:tsan --platform_suffix=tsan |
only the runtime options should get added as test config, the others are build configs!
sanitizer/sanitizer.bazelrc
Outdated
| test:asan_ubsan_lsan --test_env=ASAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:detect_leaks=1:halt_on_error=1:allocator_may_return_null=1 | ||
| test:asan_ubsan_lsan --test_env=UBSAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:print_stacktrace=1:halt_on_error=1 | ||
| test:asan_ubsan_lsan --test_env=LSAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:halt_on_error=1 |
There was a problem hiding this comment.
such runtime options should get extracted into a single place and then reused here instead
| test:asan_ubsan_lsan --test_env=ASAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:detect_leaks=1:halt_on_error=1:allocator_may_return_null=1 | |
| test:asan_ubsan_lsan --test_env=UBSAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:print_stacktrace=1:halt_on_error=1 | |
| test:asan_ubsan_lsan --test_env=LSAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:halt_on_error=1 | |
| test:_asan_runtime_options --test_env=ASAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:detect_leaks=1:halt_on_error=1:allocator_may_return_null=1 | |
| test:_asan_runtime_options --test_env=LSAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:halt_on_error=1 | |
| test:_ubsan_runtime_options --test_env=UBSAN_OPTIONS=exitcode=55:allow_addr2line=1:verbosity=1:print_stacktrace=1:halt_on_error=1 | |
| <...> | |
| test:asan_ubsan_lsan --config=_asan_runtime_options | |
| test:asan_ubsan_lsan --config=_lsan_runtime_options | |
| test:asan_ubsan_lsan --config=_ubsan_runtime_options | |
| <...> | |
| test:asan --config=_asan_runtime_options | |
There was a problem hiding this comment.
and please use the following runtime options instead:
test:_asan_runtime_options --test_env=ASAN_OPTIONS="exitcode=55 allow_addr2line=1 halt_on_error=1 print_stats=1 verbosity=1 allocator_may_return_null=1 check_initialization_order=1 detect_leaks=1 detect_stack_use_after_return=1 strict_string_checks=1"
test:_lsan_runtime_options --test_env=LSAN_OPTIONS="exitcode=55 allow_addr2line=1 halt_on_error=1 print_stats=1 verbosity=1"
test:_ubsan_runtime_options --test_env=UBSAN_OPTIONS="exitcode=55 allow_addr2line=1 halt_on_error=1 print_stacktrace=1 verbosity=1"
test:_tsan_runtime_options --test_env=TSAN_OPTIONS="exitcode=55 allow_addr2line=1 halt_on_error=1 print_stats=1 verbosity=1 detect_deadlocks=1 second_deadlock_stack=1"
.github/workflows/tests.yml
Outdated
| - name: Test with ${{ matrix.config }} | ||
| working-directory: tests | ||
| run: | | ||
| bazel test --config=${{ matrix.config }} //:sample_test --verbose_failures |
There was a problem hiding this comment.
why not
| bazel test --config=${{ matrix.config }} //:sample_test --verbose_failures | |
| bazel test --config=${{ matrix.config }} //... --verbose_failures |
?
since there will be further unit tests in this repo soon
MODULE.bazel
Outdated
|
|
||
| module( | ||
| name = "score_cpp_policies", | ||
| version = "0.0.1", |
There was a problem hiding this comment.
Can you please set the version to 0.0.0 here? The workflow in the S-CORE bazel registry is taking care of generating a patch when a release is picked.
LICENSE.md
Outdated
| @@ -0,0 +1,13 @@ | |||
| Copyright 2026 Contributors to the Eclipse Foundation | |||
There was a problem hiding this comment.
Please put the entire license text here and remove .md extension. You can copy it,for example for here: https://github.com/eclipse-score/module_template/blob/main/LICENSE
General guideline: https://www.eclipse.org/projects/handbook/#legaldoc-license
CONTRIBUTION.md
Outdated
|
|
||
| The [Eclipse Safe Open Vehicle Core (S-CORE)](https://projects.eclipse.org/projects/automotive.score) project develops an open-source core stack for Software Defined Vehicles (SDVs). This repository centralizes the shared C++ quality tool policies (sanitizers, clang-tidy, clang-format) that S-CORE modules reuse to maintain consistent, safety-focused defaults. | ||
|
|
||
| Project communication happens via the [score-dev mailing list](https://accounts.eclipse.org/mailing-list/score-dev), GitHub issues and pull requests, and the [Eclipse SCORE chatroom](https://chat.eclipse.org/#/room/#automotive.score:matrix.eclipse.org). |
There was a problem hiding this comment.
Where is the statement coming from? S-CORE is not using Eclipse matrix chat for communication, but Slack instead.
There was a problem hiding this comment.
Removed Matrix chat reference, S-CORE uses Slack
Done> |
| # ******************************************************************************* | ||
|
|
||
| name: Tests | ||
|
|
There was a problem hiding this comment.
Please limit permissions for security
| permissions: | |
| contents: read |
| @@ -0,0 +1,62 @@ | |||
| // ******************************************************************************* | |||
There was a problem hiding this comment.
Should we also have a second test that fails in sanitizers? Otherwise, how can we know that they work?
.github/workflows/tests.yml
Outdated
| disk-cache: ${{ github.workflow }}-${{ matrix.config }} | ||
| cache-save: ${{ github.event_name == 'push' }} |
There was a problem hiding this comment.
For this kind of small implementation, caching in github is superfluous IMHO. It will make the build just longer. I wouldn't introduce it yet.
But how about adding bazelisk? This one can actually help (you'll also need to add .bazelversion file(s).
| disk-cache: ${{ github.workflow }}-${{ matrix.config }} | |
| cache-save: ${{ github.event_name == 'push' }} | |
| bazelisk-cache: true | |
| cache-save: ${{ github.event_name == 'push' }} |
.github/workflows/tests.yml
Outdated
|
|
||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
| uses: actions/checkout@v4 | |
| uses: actions/checkout@v6 |
.github/workflows/tests.yml
Outdated
|
|
||
| - name: Upload test logs on failure | ||
| if: failure() | ||
| uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
| uses: actions/upload-artifact@v4 | |
| uses: actions/upload-artifact@v7 |
sanitizers/sanitizers.bazelrc
Outdated
| # ******************************************************************************* | ||
|
|
||
| # ============================================================================== | ||
| # Centralized Google Sanitizer Configurations for S-CORE C++ Modules |
There was a problem hiding this comment.
let's omit the term Google here, what do you think?
README.md
Outdated
| # Import centralized sanitizer configurations | ||
| try-import %workspace%/../../../external/score_cpp_policies/sanitizer/sanitizer.bazelrc | ||
| # Import centralized sanitizer configurations (fails if not found) | ||
| import %workspace%/external/score_cpp_policies~/sanitizers/sanitizers.bazelrc |
There was a problem hiding this comment.
~ in between directory names intended here?
There was a problem hiding this comment.
Changed to copy pattern instead of import.
.github/ISSUE_TEMPLATE/bug_report.md
Outdated
|
|
||
| ### Observed behavior: | ||
|
|
||
| ### Expected behavior |
.github/pull_request_template.md
Outdated
|
|
||
| ## Checklist for the PR Reviewer | ||
|
|
||
| * [ ] Commits are properly organized and messages are according to the guideline |
There was a problem hiding this comment.
could we link such guidelines here as a direct reference for people?
| cc_binary( | ||
| name = "asan_fail", | ||
| srcs = ["negative/asan_fail.cpp"], | ||
| tags = ["manual"], | ||
| testonly = True, | ||
| ) | ||
|
|
||
| cc_binary( | ||
| name = "lsan_fail", | ||
| srcs = ["negative/lsan_fail.cpp"], | ||
| tags = ["manual"], | ||
| testonly = True, | ||
| ) | ||
|
|
||
| cc_binary( | ||
| name = "tsan_fail", | ||
| srcs = ["negative/tsan_fail.cpp"], | ||
| tags = ["manual"], | ||
| testonly = True, | ||
| ) | ||
|
|
||
| cc_binary( | ||
| name = "ubsan_fail", | ||
| srcs = ["negative/ubsan_fail.cpp"], | ||
| tags = ["manual"], | ||
| testonly = True, | ||
| ) |
There was a problem hiding this comment.
you should wrap an sh script around the invocation of each test instead and then add them as sh_test here. Within the sh script, you would have to verify that the test executable fails with exitcode 55.
Do you get my my idea?
Since then, you can remove the manual tags and have these tests running in CI as well ;)
There was a problem hiding this comment.
Thanks for your suggestion. Done.
sanitizers/BUILD.bazel
Outdated
| # ******************************************************************************* | ||
|
|
||
| exports_files( | ||
| ["sanitizer.bazelrc"], |
There was a problem hiding this comment.
| ["sanitizer.bazelrc"], | |
| ["sanitizers.bazelrc"], |
| @@ -0,0 +1 @@ | |||
| 8.3.0 | |||
There was a problem hiding this comment.
do we really want to have the tests as separate module?
There was a problem hiding this comment.
why is this required here?
Added per @4og 's feedback to enable bazelisk caching
There was a problem hiding this comment.
do we really want to have the tests as separate module?
Yes, this follows the standard SCORE pattern used in score_rust_policies and other policy repos.
| # ******************************************************************************* | ||
| # Copyright (c) 2026 Contributors to the Eclipse Foundation | ||
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # ******************************************************************************* |
There was a problem hiding this comment.
why is this license statement different from others?
tests/BUILD.bazel
Outdated
| # ============================================================================== | ||
|
|
||
| cc_binary( | ||
| name = "asan_fail_bin", |
There was a problem hiding this comment.
why not
| name = "asan_fail_bin", | |
| name = "asan_fail_test", |
since _bin might be misleading
README.md
Outdated
| ``` | ||
|
|
||
| Add to your module's `.bazelrc`: | ||
| Copy these configurations to your project's `.bazelrc`: |
There was a problem hiding this comment.
NO, I have to object here. we need to find a mechsnism to make this work centrally, e.g. via run_under and using a centralized script then. otherwise the settings we define here will simply get duplicated everywhere and these will deviate over time then which is not maintainable at all!
There was a problem hiding this comment.
Agree. The whole point of this repository is to avoid duplication of sanitizer config, and now the README is just asking to duplicate 🤦♂️
There was a problem hiding this comment.
Added --run_under as suggested.
789a32e to
0413906
Compare
0413906 to
d6f8104
Compare
1566f5a to
07a78d6
Compare
45cf3d1 to
b204ecd
Compare
b204ecd to
041c5a9
Compare
| load("@rules_cc//cc/toolchains:feature.bzl", "cc_feature") | ||
|
|
||
| # Link flags for ASan + UBSan + LSan combined | ||
| cc_args( |
There was a problem hiding this comment.
fsanitize options must be provided to the compile and link step! Compiler instruments code, linker adds runtime dependencies of sanitizer. One without the other does not work.
| @@ -0,0 +1,3 @@ | |||
| ASAN_OPTIONS=exitcode=55 allow_addr2line=1 verbosity=1 check_initialization_order=1 detect_stack_use_after_return=1 print_stats=1 halt_on_error=1 allocator_may_return_null=1 detect_leaks=1 suppressions=%ROOT%sanitizers/suppressions/asan.supp | |||
There was a problem hiding this comment.
how about adding strict_string_checks=1 to ASAN_OPTIONS here?
There was a problem hiding this comment.
and maybe sort all options alphabetical to better find a certain setting?
MODULE.bazel
Outdated
|
|
||
| module( | ||
| name = "score_cpp_policies", | ||
| version = "0.0.0", |
There was a problem hiding this comment.
Now the registry allows empty versions.
This can be reduced to module(name = "score_cpp_policies")
There was a problem hiding this comment.
Done. simplified to module(name = "score_cpp_policies")
9beb9a1 to
6a56979
Compare
6a56979 to
bf16a68
Compare
.github/ISSUE_TEMPLATE/bug_fix.md
Outdated
| @@ -0,0 +1,11 @@ | |||
| --- | |||
There was a problem hiding this comment.
There are now org-wide templates for issues: https://github.com/eclipse-score/.github/tree/main/.github/ISSUE_TEMPLATE
I suggest removing custom templates for issues from this PR.
c482919 to
bbd378c
Compare
| remote = "https://github.com/bazel-contrib/toolchains_llvm", | ||
| ) | ||
|
|
||
| bazel_dep(name = "score_cpp_policies", version = "") |
|
|
||
| module( | ||
| name = "score_cpp_policies_tests", | ||
| version = "0.0.0", |
| build:tsan --features=tsan | ||
| build:tsan --platform_suffix=tsan | ||
| test:tsan --config=with_debug_symbols | ||
| test:tsan --cxxopt=-O1 |
There was a problem hiding this comment.
should get removed again here since you added it in the build action features above already
Summary
Adds centralized sanitizer configurations (ASan, TSan, UBSan, LSan) for S-CORE C++ projects as a reusable Bazel module.
What's Included
Usage