{{ message }}
fix(rules-sycl): name the C library, which the rule never did (0.2.1) - #4
Merged
Conversation
The rule names the C++ standard library because, left alone, dpcpp's clang
reads the host's. The C library is the same question one layer down and had no
answer. Measured on dpcpp 7.1.0 with the flags the rule passed:
…/xim-x-gcc/15.1.0/…/include/c++/15.1.0 <- ecosystem, correct
…/xim-x-dpcpp/7.1.0/lib/clang/22/include
/usr/local/include <- the HOST
/usr/include/x86_64-linux-gnu
/usr/include
No ecosystem glibc path anywhere on the list. So `<cstdio>` in a `.sycl` unit
reached libstdc++ from the payload and `<stdio.h>` from the host.
FORWARDING THE SYSROOT DOES NOT ANSWER IT HERE, which is why the documented fix
for a second compiler does not apply. `toolchain_sysroot()` is EMPTY under an
llvm toolchain -- measured -- and a SYCL project pins llvm because mcpp's own
clang has no SYCL front end. The LLVM payload's clang does not need it, being
configured with the ecosystem glibc; dpcpp's clang is a different clang and is
not. So the rule names the C library the way it already names the C++ one, and
requires `xim:glibc` and `xim:linux-headers` as it already requires `xim:gcc`.
THE OLD CRITERION COULD NOT SEE THIS, and its reasoning says why. It asserts
"no host C++ standard library" rather than "no /usr", because the engine's own
compiles leave `/usr/include` last as a fallback for C headers -- true, and it
holds only when the ecosystem's C library is AHEAD of it. For dpcpp's clang
`/usr/include` was not a fallback, it was the only source.
The new assertion is therefore about ORDER, not absence: every device compile
must either name the ecosystem glibc or forward a sysroot. Measured across all
three fixtures; the SYCL one satisfies it only after this change, and the
ecosystem glibc moves from absent to third on the list.
Found by enumerating the rules after the same defect class broke llama.cpp-m's
CI, not by a failure. It ships in 0.2.0, so this is 0.2.1.
The first form grepped the build log, and the build log carries the ENGINE's
compile lines -- which do name the ecosystem glibc. So it passed for a rule
whose device compile named nothing, and removing the fix it was written for
left it green. Verified, which is the only reason it is not still that shape.
Two things it now gets right:
* THE OBJECT. An implicit include search never appears on a command line, and
the log mixes compiles from different compilers. `tools/check_device_c_library.sh`
extracts the device compile's own C-library flags from build.ninja, asks
that compiler to print its `#include <...>` search list, and reads the
order.
* THE PATTERN. "Any ecosystem path" matched dpcpp's own `include/`, because a
compiler's resource directory lives in the store too. It now matches what
actually provides `features.h`: the glibc payload, or a sysroot's
`usr/include` beneath the registry.
Two admissible answers, both the same property from either side: the rule names
the C library, or the compiler it drives already knows it. Refused is a device
compile that reaches a host path with no ecosystem C library above it.
Measured across all three fixtures -- cuda at position 4, hip at 4, sycl at 2,
first host path at 8, 8 and 6 -- and verified to FAIL for sycl with the fix
removed.
…roject does not choose its version The first form asked for `"xim:glibc" = "2.44"`, and CI refused the fixture that declared exactly that: the runner's runtime binding had chosen glibc 2.44.2. `xpkg_dir` with a pin answers for that version or for nothing, so the rule told a project to declare something it had already declared. The C library version is the RUNTIME BINDING's choice. `""` -- present, any version -- is the only thing a project can truthfully say about a library it does not select, and it resolves to whatever the binding installed. Measured: `"xim:glibc" = ""` answers `…/xim-x-glibc/2.44.2` on the machine that has 2.44.2 in its store. This is the same shape as the defect the round found in mcpp itself: a value that belongs to one layer being spelled by another.
It read whatever `build.ninja` sorted first under a fixture's target, which is
a guess about what a previous step left. The fixture steps in CI end with
`build --no-accel`, whose graph has no device action at all, so the check
reported "declares no action to inspect" for three fixtures that were correct.
Two changes, both removing a guess:
* it runs the accel build itself, because that build is what it is about;
* it selects the graph that CONTAINS `rule mcpp_action_`, rather than the one
that sorts first.
It also prints which build.ninja it read, so a future disagreement names its
object instead of being inferred.
Fourth time in this round that a check chose its own object and chose wrong,
and the shape is identical every time: a path or a file picked by convention
rather than by the property being measured.
Verified against the CI shape locally -- accel build followed by `--no-accel`,
then the check -- and still FAILS for sycl with the fix removed.
Carrying the rule's language machinery into the probe was a mistake with a silent shape: the CUDA and HIP commands begin with `-x cuda`, the probe added its own `-x c++`, and a compiler given two of them prints no search list at all. The check reported that as a defect in three fixtures, two of which were correct. Where `features.h` comes from does not depend on the language being compiled, so the probe no longer names one. It keeps `--sysroot=`, `--gcc-install-dir=` and `-isystem` -- the flags that move the C library -- and asks in plain C++. It also reads the compiler's stderr, because `-v` writes the search list there, and prints what the compiler said when there is still nothing to read. The previous form could only say "printed no search list", which is a description of the check rather than of the problem. Measured: cuda 4/7, hip 4/7, sycl 2/8 (ecosystem C library position, first host path), and still FAILS for sycl with the rule's fix removed.
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.

What
mcpp.rules.syclnames the C++ standard library because, left alone, dpcpp'sclang reads the host's. The C library is the same question one layer down and
had no answer.
Measured
The dpcpp clang's include search list, with the flags the rule passed:
No ecosystem glibc path anywhere on it. So
<cstdio>in a.syclunit reachedlibstdc++ from the payload and
<stdio.h>from the host.After the fix, the ecosystem glibc is third and every host path sits below
the eleventh.
Forwarding the sysroot does not answer it here
mcpp::toolchain_sysroot()is the documented answer for a second compiler, andit is empty under an llvm toolchain — measured — which is what a SYCL
project pins, because mcpp's own clang has no SYCL front end. The LLVM payload's
clang does not need it, being configured with the ecosystem glibc; dpcpp's
clang is a different clang and is not.
So the rule names the C library the way it already names the C++ one, and
requires
xim:glibcandxim:linux-headersas it already requiresxim:gcc.The old criterion could not see this, and its own reasoning says why
The existing assertion is
no host C++ standard libraryrather thanno /usr,because the engine's own compiles leave
/usr/includelast as a fallback for Cheaders. True — and it holds only when the ecosystem's C library is ahead of
it. For dpcpp's clang
/usr/includewas not a fallback, it was the onlysource.
The new assertion is therefore about order, not absence: every device
compile must either name the ecosystem glibc or forward a sysroot. Measured
across all three fixtures; the SYCL one satisfies it only after this change.
How it was found
By enumerating the rules after the same defect class broke llama.cpp-m's CI —
not by a failure.
rules-spirvdrives GLSL compilers and has no C library;rules-cudaalready forwards the sysroot;rules-hipdrives the LLVM payload'sclang, whose search list carries the ecosystem glibc first (verified).
Ships in 0.2.0, so this is 0.2.1.