-
Notifications
You must be signed in to change notification settings - Fork 0
Add sanitizer support using GCC toolchain features #3
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
base: codex/replay-pr-64-base
Are you sure you want to change the base?
Changes from all commits
e79bcb1
d2c40e7
1ede9eb
5bc82b8
f70f1c0
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 @@ | ||
| # ******************************************************************************* | ||
| # 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] | ||
|
|
||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/[email protected] | ||
| 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/[email protected] | ||
| with: | ||
| bazelisk-version: 1.26.0 | ||
| disk-cache: true | ||
| repository-cache: true | ||
| bazelisk-cache: true | ||
| cache-save: ${{ github.event_name == 'push' }} | ||
|
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. [Logic] Context for AgentsChain of Thought
Reasoning Failed to parse validation response Evidence Summary 1. File Lookup
2. File Lookup
3. Library Docs
4. File Lookup
5. File Snapshot (HEAD)
6. File Lookup
7. File Lookup
8. File Lookup
9. File Lookup
10. File Snapshot (BASE)
|
||
|
|
||
| - name: Run sanitizer tests via Bazel | ||
| run: | | ||
| 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 \ | ||
|
Comment on lines
+56
to
+60
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. [Testing] Passing Context for AgentsChain of Thought
Reasoning The review comment claims:
Evidence gathered:
The ... Evidence Summary 1. File Snapshot (HEAD)
|
||
| --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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -37,6 +37,12 @@ bazel_dep(name = "score_platform", version = "0.5.3", dev_dependency = True) | |||||
| # Toolchains and extensions | ||||||
| bazel_dep(name = "score_bazel_cpp_toolchains", version = "0.2.2", dev_dependency = True) | ||||||
| bazel_dep(name = "score_toolchains_rust", version = "0.4.0", dev_dependency = True) | ||||||
| bazel_dep(name = "score_cpp_policies", version = "0.0.0", dev_dependency = True) | ||||||
| git_override( | ||||||
| module_name = "score_cpp_policies", | ||||||
| commit = "07a78d6", | ||||||
| remote = "https://github.com/RSingh1511/score_cpp_policies.git", | ||||||
|
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. [Security] The remote points to a personal GitHub repository (
Suggested change
Context for AgentsChain of Thought
Reasoning The comment is accurate and well-supported by evidence:
Evidence Summary 1. File Snapshot (HEAD)
2. File Snapshot (BASE)
|
||||||
| ) | ||||||
|
|
||||||
| # 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( | ||
|
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. [Logic] The Context for AgentsChain of Thought
Reasoning The comment correctly identifies that:
The one uncertainty is whether the external However, the comment is phrased as a question ("Are these suppression files actually wired into the sanitizer runtime environment?") rather than an assertion, which is appropriate given the uncertainty. It's asking the PR author to confirm the wiring exists somewhere (perhaps in the external wrapper). The comment is actionable because it:
Currently, the suppression files are empty (just comments), so the practical impact is zero right now. But the comment is valid as a design review concern about the infrastructure being incomplete. Evidence Summary 1. File Snapshot (HEAD)
2. File Snapshot (HEAD)
3. File Snapshot (HEAD)
4. File Snapshot (HEAD)
5. File Snapshot (HEAD)
6. File Snapshot (HEAD)
7. File Snapshot (HEAD)
|
||
| 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.
[Logic] The shortcut configs (
asan,ubsan,lsan) inheritasan_ubsan_lsanvia--config=asan_ubsan_lsanbut then override--test_tag_filterswith a narrower filter. Since Bazel uses last-value-wins for--test_tag_filters, runningbazel test --config=asanwill build with ASan+UBSan+LSan but only exclude-no-asantests, not-no-lsanor-no-ubsan. This means tests taggedno-ubsanorno-lsanwill still execute under those sanitizers, potentially causing false failures or undefined behavior.Either remove the shortcut tag overrides (relying on the parent config's filters) or make the shortcuts self-contained with the correct combined filters:
Context for Agents
Chain of Thought
Reasoning
The comment correctly identifies a real bug in the newly added code. Here's the evidence:
Bazel's
--test_tag_filtersis a last-value-wins flag - confirmed via library docs. When specified multiple times, the last value completely replaces earlier values.The config expansion chain creates a conflict:
build:asan --config=asan_ubsan_lsan(line 106) inherits the parent config, which includestest:asan_ubsan_lsan --test_tag_filters=-no-asan,-no-lsan,-no-ubsan(line 101)test:asan --test_tag_filters=-no-asan(line 107) then overrides the inherited test_tag_filters with a narrower filterNet effect of
--config=asan:no-asan, NOTno-ubsanorno-lsanno-ubsanorno-lsanwill still execute under those active sanitizersThis is newly introduced by this PR - all the sanitizer config lines are additions in the diff.
The comment is actionable - it provides two concrete solutions: either remove the shortcut tag overrides entirely (relying on parent's filters) or make each shortcut use the full combined filter string.
The same issue applies symmetrically to
--config=ubsanand--config=lsan.Evidence Summary
1. Library Docs
bazeltest_tag_filters flag behavior last value wins accumulating2. File Snapshot (HEAD)
.bazelrc85-124