ASan TSan#196
Merged
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
docs/BUILD_SYSTEM.md — new section “AddressSanitizer and ThreadSanitizer (macOS)” covering: The three-way clang major coupling (MODULE.bazel, Xcode, .bazelrc TSAN rpath) Why ASAN uses the full Xcode toolchain vs TSAN using LLVM compile + Xcode runtime Steps when upgrading LLVM or Xcode Smoke-test commands Also corrected the outdated LLVM version note (darwin was listed as 20.1.8; it’s 21.1.8). .bazelrc — short pointers to BUILD_SYSTEM.md; TSAN rpath comment notes the hardcoded clang/21 must stay in sync. MODULE.bazel — comment on llvm_versions to update the TSAN rpath and verify Xcode when bumping LLVM.
Collaborator
|
This looks useful. Xcode's llvm version was far too old when I first tried to add the ASAN and TSAN options. |
…arking global arrays as static. Commented out unused SORT_SOLVE_STRENGTH and SORT_SOLVE_STRENGTH_CUTOFF
The BUILD change ensures a stats test links only system_util_stats, not system + system_util_stats. One system variant → one ThreadMgr → one set of mutexes. That matches the intent: a process-wide thread manager.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes macOS sanitizer (ASan/TSan) builds by switching to Bazel --config-based sanitizer wiring, selecting an Xcode toolchain for ASan, and documenting/version-coupling the macOS runtime requirements.
Changes:
- Add
apple_support(Bzlmod) and wire an Apple toolchain repo for macOS ASan builds; update.bazelrcASan/TSan configs accordingly. - Remove legacy
--define=asan=trueplumbing and update docs/instructions to use--config=asan/--config=tsan. - Add Linux/macOS sanitizer CI coverage and adjust internal build targets to avoid linking mismatched
systemvariants.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| MODULE.bazel.lock | Updates module lock to reflect new dependency graph. |
| MODULE.bazel | Adds apple_support and configures Apple toolchain repos; documents clang-major coupling. |
| .bazelrc | Reworks ASan/TSan configs for macOS (Xcode toolchain for ASan; Xcode runtime rpath for TSan). |
| BUILD.bazel | Removes legacy //:asan config_setting. |
| CPPVARIABLES.bzl | Removes --define=asan=true-based sanitizer flags from global opts/linkopts. |
| docs/BUILD_SYSTEM.md | Documents macOS ASan/TSan behavior, coupling points, and CI commands. |
| .github/copilot-instructions.md | Updates guidance to use --config=asan. |
| .github/workflows/ci_linux.yml | Adds ASan/TSan jobs and timeouts; uploads logs. |
| .github/workflows/ci_macos.yml | Adds sanitizer job and timeouts; uploads logs. |
| .github/workflows/ci_windows.yml | Adds job timeout. |
| .github/workflows/ci_wasm.yml | Adds job timeout. |
| library/src/system/BUILD.bazel | Clarifies that multiple system variants share sources and shouldn’t be co-linked. |
| library/src/solver_context/BUILD.bazel | Switches log/stats variants to depend on corresponding system variants. |
| library/src/system/thread_mgr.cpp | Moves global mutexes into an anonymous namespace for internal linkage. |
| library/src/system/scheduler.cpp | Gives internal linkage to scheduling constant tables (avoids cross-TU symbol collisions). |
| library/src/moves/moves.cpp | Zero-initializes moveList entries in constructor to avoid uninitialized state. |
| library/src/heuristic_sorting/heuristic_sorting.cpp | Minor conditional adjustment for g boundary check. |
Collaborator
Author
|
This PR includes several sets of commits that could be submitted separately:
It was expedient to do this together. Let me know if you'd rather see separate PRs for each. |
zzcgumn
reviewed
Jun 25, 2026
| { 1.525, 1.810, 0.0285 }, | ||
| { 1.585, 1.940, 0.0354 } | ||
| }; | ||
| // #define SORT_SOLVE_STRENGTH_CUTOFF 0 |
Collaborator
There was a problem hiding this comment.
We should either the deleted the commented out code or explain why we are keeping it.
Collaborator
Author
There was a problem hiding this comment.
Agreed. That seems orthogonal to this PR. Shall I add an issue for it?
Collaborator
Author
There was a problem hiding this comment.
Also, do you have any reason to keep it?
Collaborator
There was a problem hiding this comment.
No reason to keep it. We can create a new issue to keep this PR leaner.
Collaborator
|
This looks good in general, one small comment from me. I noticed that the building with the ASAN/TSAN flags picked up a couple issues. |
Collaborator
Author
|
Perhaps I’m confused. Isn’t the PR leaner if we leave this code as is? I’m
away from my laptop, so not seeing the full context.
…On Thu, Jun 25, 2026 at 11:44 AM Martin Nygren ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In library/src/system/scheduler.cpp
<#196 (comment)>:
> {
{ 284000, 91000, 37000, 23000, 17000, 15000, 13000, 4000 },
{ 388000, 140000, 60000, 40000, 30000, 23000, 18000, 6000 },
};
-#define SORT_SOLVE_STRENGTH_CUTOFF 0
-
-double SORT_SOLVE_STRENGTH[2][3] =
-{
- { 1.525, 1.810, 0.0285 },
- { 1.585, 1.940, 0.0354 }
-};
+// #define SORT_SOLVE_STRENGTH_CUTOFF 0
No reason to keep it. We can create a new issue to keep this PR leaner.
—
Reply to this email directly, view it on GitHub
<#196?email_source=notifications&email_token=ABC4PYAFJJ5JTDK67V2MUDL5BVJGHA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJXGMYDMMZYGM3KM4TFMFZW63VGMFZXG2LHN2SWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#discussion_r3476087675>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABC4PYD6SIYEQ2O22RQBWUD5BVJGHAVCNFSNUABEKJSXA33TNF2G64TZHMZDMOJYHAZDANR3JFZXG5LFHM2DMOJVGA4TIMRYGCQXMAQ>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix
ASan(AddressSanitizer) andTSan(ThreadSanitizer) builds on macOS. These are used for testing.