-
Notifications
You must be signed in to change notification settings - Fork 224
bazel: add debug/sanitizer configs and expand build documentation #3538
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: main
Are you sure you want to change the base?
Changes from all commits
34b4991
9204232
9fa277e
04b6db8
a5f1714
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -110,3 +110,122 @@ build:dpc-private \ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| test:dpc-private \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| --test_tag_filters="dpc,-public" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Configuration: 'release-dpc' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Build release artifacts including DPC++ libs. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The //:release target automatically builds all ISA variants when | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # --cpu is unset (auto). Use --cpu=<isa> to restrict for CI speed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # bazel build //:release # CPU only, all ISAs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # bazel build //:release --config=release-dpc # with DPC++ libs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # bazel build //:release --cpu=avx2 # CI: avx2 only | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+117
to
+121
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The //:release target automatically builds all ISA variants when | |
| # --cpu is unset (auto). Use --cpu=<isa> to restrict for CI speed. | |
| # bazel build //:release # CPU only, all ISAs | |
| # bazel build //:release --config=release-dpc # with DPC++ libs | |
| # bazel build //:release --cpu=avx2 # CI: avx2 only | |
| # The //:release target builds multiple ISA variants when | |
| # --cpu is set to 'all' (or another multi-ISA setting). By default, | |
| # @config//:cpu is 'auto', which selects a single host ISA. | |
| # bazel build //:release --cpu=all # CPU only, all ISAs | |
| # bazel build //:release --config=release-dpc --cpu=all # with DPC++ libs | |
| # bazel build //:release --cpu=avx2 # CI: avx2 only |
Copilot
AI
Mar 2, 2026
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.
--release_dpc=true uses a different boolean spelling/casing than the other config flags in this repo (e.g., config settings compare against "True"). For consistency (and to avoid any ambiguity in bool parsing), use True/False here as well.
| --release_dpc=true | |
| --release_dpc=True |
Copilot
AI
Mar 2, 2026
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.
The dev config description says it “compiles only for the host machine's highest ISA”, but --cpu=auto still includes the sse2 baseline in addition to the detected host ISA (per cpu_info implementation). Please adjust the wording so developers aren’t surprised by the extra ISA being built.
| # Fast local development build — compiles only for the host machine's | |
| # highest ISA (auto-detected). Use this to speed up iterative builds. | |
| # NOT suitable for release/packaging. | |
| # Fast local development build — compiles for the host machine's | |
| # highest ISA (auto-detected), plus the baseline sse2 variant. Use this | |
| # to speed up iterative builds. NOT suitable for release/packaging. |
Copilot
AI
Mar 7, 2026
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.
Using common:<config> means these options will also be applied to non-build commands when invoked with --config=... (e.g., bazel query), which can cause “unknown option” failures for flags like --compilation_mode, --copt, and --linkopt. To keep configs usable across Bazel subcommands, consider defining these as build:<config> and test:<config> (duplicated), rather than common:<config>.
| common:dbg \ | |
| --compilation_mode=dbg \ | |
| --enable_assert=True | |
| # Configuration: 'dbg-symbols' | |
| # Debug symbols only — no assertion checking, no -O0. | |
| # Equivalent to Make: REQDBG=symbols | |
| # Adds -g to the default opt build; preserves -O2/-O3 optimization. | |
| # bazel build //:release --config=dbg-symbols | |
| common:dbg-symbols \ | |
| --copt=-g \ | |
| --cxxopt=-g | |
| build:dbg \ | |
| --compilation_mode=dbg \ | |
| --enable_assert=True | |
| test:dbg \ | |
| --compilation_mode=dbg \ | |
| --enable_assert=True | |
| # Configuration: 'dbg-symbols' | |
| # Debug symbols only — no assertion checking, no -O0. | |
| # Equivalent to Make: REQDBG=symbols | |
| # Adds -g to the default opt build; preserves -O2/-O3 optimization. | |
| # bazel build //:release --config=dbg-symbols | |
| build:dbg-symbols \ | |
| --copt=-g \ | |
| --cxxopt=-g | |
| test:dbg-symbols \ | |
| --copt=-g \ | |
| --cxxopt=-g |
Copilot
AI
Mar 7, 2026
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.
--config=type uses -fsanitize=type, which is Clang-only (GCC doesn’t support TypeSanitizer). Consider documenting that this config requires Clang/ICPX (similar to the MSan note), so users on GCC don’t get a confusing compiler error.
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -373,3 +373,160 @@ dal_test_suite( | |||||||||||||||
|
|
||||||||||||||||
| ## What is missing in this guide | ||||||||||||||||
| - How to get make-like release structure | ||||||||||||||||
|
|
||||||||||||||||
| ## Debug and Sanitizer Builds | ||||||||||||||||
|
|
||||||||||||||||
| ### Debug build with assertions | ||||||||||||||||
|
|
||||||||||||||||
| Equivalent to Make `REQDBG=1` — adds debug symbols, enables `DEBUG_ASSERT` | ||||||||||||||||
| and `ONEDAL_ENABLE_ASSERT`: | ||||||||||||||||
|
|
||||||||||||||||
| ```sh | ||||||||||||||||
| bazel build //:release --config=dbg | ||||||||||||||||
| bazel test //cpp/oneapi/dal:tests --config=dbg | ||||||||||||||||
| ``` | ||||||||||||||||
|
Comment on lines
+384
to
+387
|
||||||||||||||||
|
|
||||||||||||||||
| ### Debug symbols only (no assertion checks) | ||||||||||||||||
|
|
||||||||||||||||
| Equivalent to Make `REQDBG=symbols`: | ||||||||||||||||
|
|
||||||||||||||||
| ```sh | ||||||||||||||||
| bazel build //:release --config=dbg-symbols | ||||||||||||||||
| ``` | ||||||||||||||||
|
|
||||||||||||||||
| > Note: The `libonedal_dpc.so` library does not support debug mode due to | ||||||||||||||||
|
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. This will change in one of the PRs that is in progress.
Contributor
Author
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. Acknowledged. I adjusted the wording to avoid hard dependency on that in-flight PR and kept the note generic. |
||||||||||||||||
| > excessive debug information causing long link times. Use the static library | ||||||||||||||||
| > variant for DPC++ debugging. | ||||||||||||||||
|
Comment on lines
+397
to
+399
|
||||||||||||||||
| > Note: The `libonedal_dpc.so` library does not support debug mode due to | |
| > excessive debug information causing long link times. Use the static library | |
| > variant for DPC++ debugging. | |
| > Note: Debug builds of the `libonedal_dpc.so` shared library may produce | |
| > excessive debug information and result in very long link times. For | |
| > practical DPC++ debugging, it is recommended to use the static library | |
| > variant instead. |
Copilot
AI
Mar 7, 2026
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.
The Type Sanitizer section doesn’t mention compiler requirements. -fsanitize=type is Clang-only; GCC users will see a hard failure. Consider adding a short note (like the MSan section) indicating this requires Clang/ICPX and may be unavailable depending on the toolchain.
| Requires Clang or ICPX. GCC does not support the Type Sanitizer and availability may depend on your toolchain. |
Copilot
AI
Mar 7, 2026
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.
This section says bazel build //:release builds “all ISA variants” and that the default --cpu=auto triggers that. In the current Bazel config, @config//:cpu defaults to auto, which selects a single auto-detected ISA. Please update the instructions to use --cpu=all (or modern / explicit ISA list) when you mean “all ISA variants”, and clarify what auto does.
Copilot
AI
Mar 2, 2026
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.
The README claims //:release “automatically compiles all ISA variants” when --cpu is left at its default (auto). In the current Bazel config, @config//:cpu defaults to auto, which expands to sse2 + the detected host ISA (not all ISAs), and there’s no transition in the repo that forces all for //:release. Please update this section to match the actual behavior (e.g., document --cpu=all for full ISA coverage) or add the missing transition if that’s the intent.
Copilot
AI
Mar 2, 2026
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.
In the Make→Bazel reference table, the row stating “Transition forces all ISAs” for bazel build //:release doesn’t match the current implementation (no ISA-forcing transition present; default --cpu=auto builds sse2 + host ISA). Please correct the note to avoid misleading users.
| | (Make default all ISAs) | `bazel build //:release` | Transition forces all ISAs | | |
| | (Make default all ISAs) | `bazel build //:release` | Default `--cpu=auto` builds `sse2` + host ISA | |
Copilot
AI
Mar 7, 2026
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.
The “Make default all ISAs” mapping here assumes bazel build //:release forces all ISA builds, but the current --cpu default (auto) is single-ISA. Update this table row to reflect the actual flag needed for all-ISA coverage (e.g., --cpu=all).
| | (Make default all ISAs) | `bazel build //:release` | Transition forces all ISAs | | |
| | (Make default all ISAs) | `bazel build //:release --cpu=all` | Transition forces all ISAs | |
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.
These comments state that
//:releasebuilds “all ISA variants” when--cpuis unset/auto. In the current config,--cpu=autoexpands tosse2+ the detected host ISA, not all ISAs. Please update the comment/examples to match reality (or switch examples to--cpu=allif the intent is full ISA coverage).