Repository navigation
fix(sycl): propagate -fsycl via sycl_dependency to fix test SIGSEGV (ADR-1099) - #825
Merged
Merged
Conversation
lusoris
force-pushed
the
worktree-wf_2dbe208f-ac9-3
branch
from
June 7, 2026 02:02
0f516e7 to
6f54b40
Compare
…ADR-1099) Root-cause investigation of test_sycl_motion_add_uv_parity SIGSEGV after two prior incomplete fix attempts (PRs #768, #796, ADR-1093). Two root causes identified and fixed: 1. Missing -fsycl at link time for SYCL test executables. Without -fsycl, icpx skips clang-offload-wrapper and the SYCL runtime's ProgramManager never registers the device kernels from libvmaf.a. At runtime, the first queue.submit() null-dereferences inside ProgramManager::getDeviceKernelInfo (SIGSEGV). Fix: move -fsycl from vmaf_link_args (library-only) into sycl_dependency.link_args in core/src/meson.build so every target that declares dependencies: [sycl_dependency] — libvmaf.so and all SYCL test executables — gets the flag automatically. 2. Wrong feature names in vmaf_feature_score_at_index queries. With motion_add_uv=true (non-default), the feature-name system stores scores under aliased names: integer_motion2_mau (SYCL) and float_motion2_mau (CPU). The test queried the raw VMAF_*_score names, getting -EINVAL for every lookup. Fixed by using the aliased names. Also included: remove extern "C" wrappers around C++ standard library includes in all 23 SYCL TUs (GCC 15 + icpx incompatibility), and add picture_copy.h so they are safe to include from C++ without extern "C" wrappers. Removes should_fail: true from test_sycl_motion_add_uv_parity (ADR-1093 bypass reverted for this test). Adds AGENTS.md invariant notes for the -fsycl link requirement and the feature-name aliasing contract. ADR-1099, T-SYCL-MOTION-ADD-UV-SIGSEGV-2026-06-07 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lusoris
force-pushed
the
worktree-wf_2dbe208f-ac9-3
branch
from
June 7, 2026 02:04
6f54b40 to
4f004fd
Compare
lusoris
marked this pull request as ready for review
June 7, 2026 02:04
lusoris
added a commit
that referenced
this pull request
Jun 8, 2026
…ts + ffmpeg patch style (#847) * chore(changelog): backfill 30 missing fragments for PRs #806–#846 Adds one-line changelog.d fragments for 30 merged PRs in the #806–#846 range that shipped without a fragment. PRs #805, #807, and #825 were already covered by existing ADR-numbered fragments (1090, 1093, 1099); PRs #836, #839, and #840 were covered by topic-named fragments (go-rust-ci-red-bundle, cuda-done-path-double-unref-ort-coverage, 0840-pic-pool-odr-cuda-gpumask-cov-floor). All 30 new files verified by scripts/release/concat-changelog-fragments.sh with no new warnings (pre-existing chore/perf/refactor/ section warnings are unrelated to this change). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(coverage): ratchet ort_backend.c floor back to 83% (PR #844 error-injection completed) PR #840 lowered the ort_backend.c per-file coverage floor from 83 to 79 because the ADR-0922 target was above the structural ceiling: ORT error paths are unreachable without error injection. PR #844 added 8 ORT error-injection tests, bringing measured coverage to 84%. The floor is now safely ratcheted back to 83 (1 pp slack against 84% measured). Also updates the T-COVERAGE-ORT-FLOOR-OVERSHOOT-2026-06-08 row in docs/state.md to reflect the completed two-step fix (PR #840 temporary reset + PR #844 error-injection + this ratchet). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(ci): raise pytest timeouts for coverage-gpu/mcp-smoke/dnn (avoid silent hangs) coverage-gpu: add --timeout=300 --timeout-method=signal to CUDA and SYCL pytest calls (previously bare; GPU kernel init can stall silently within 12-min budget). mcp-smoke: add --timeout=60 --timeout-method=signal to socket-opening tests (previously bare; 12-min job budget gives no per-test guard). dnn: add --timeout=60 --timeout-method=signal (non-critical hygiene). Also install pytest-timeout in the dnn, mcp-smoke, and coverage-gpu venvs. coverage job --timeout=180 already correct (PR #840 precedent, unchanged). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(ffmpeg-patches): align 0010 include path to <libvmaf/libvmaf_cuda.h> (matches SYCL/HIP/Metal pattern) Patch 0010 used bare `libvmaf_cuda.h` in both the configure pkg_config probe lines and the C #include guard in vf_libvmaf.c. Every other backend patch (0003 SYCL, 0011 HIP, 0012/0013 Metal) uses the subdirectory-prefixed form `libvmaf/<name>.h`, which is the installed path under the include prefix that pkg-config and compilers actually see. The bare form works today only because the CUDA header happens to be installed both ways on the current dev machine; it would silently break on a stricter sysroot (e.g. the container build, a cross-compile host) where only `libvmaf/libvmaf_cuda.h` is present. Three occurrences updated (no functional change, style-consistency fix): configure: -enabled libvmaf ... libvmaf_cuda.h → libvmaf/libvmaf_cuda.h configure: +enabled libvmaf_cuda ... libvmaf_cuda.h → libvmaf/libvmaf_cuda.h configure: +enabled libvmaf ... libvmaf_cuda.h → libvmaf/libvmaf_cuda.h vf_libvmaf.c: #include <libvmaf_cuda.h> → #include <libvmaf/libvmaf_cuda.h> Series-replay verification (git am --3way against pristine n8.1) is deferred; patch 0010 is not standalone-applicable — it builds on 0001-0009. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Lusoris <lusoris@pm.me> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
Root-cause investigation and real fix for
test_sycl_motion_add_uv_paritySIGSEGV.Prior fix attempts (PRs #768, #796) were incomplete — test remained marked
should_fail: true(ADR-1093).Two root causes identified and fixed:
Missing
-fsyclat test-binary link time —core/src/meson.buildonlyadded
-fsycltovmaf_link_args(library-only). Test executables linkinglibvmaf.aviasycl_dependencygot no-fsycl, soclang-offload-wrapperwas skipped,
ProgramManagernever registered device kernels, and the firstqueue.submit()null-dereferenced insidegetDeviceKernelInfo(SIGSEGV).Fix: embed
-fsyclinsycl_dependency.link_argsso every consumer gets it.Wrong feature names in score queries — with
motion_add_uv=true,the feature-name system stores scores under aliased names (
integer_motion2_mau,float_motion2_mau). The test queried the rawVMAF_*_scorenames, getting-EINVALon every lookup.Additional inclusions:
extern "C"wrappers around C++ standard headers in all 23 SYCL TUs(GCC 15 + icpx incompatibility fix from prior session)
#ifdef __cplusplusguards tofeature_name.h,luminance_tools.h,picture_copy.hAGENTS.mdinvariant notes for the-fsycllink requirement and feature-namealiasing contract
Removes
should_fail: truefromtest_sycl_motion_add_uv_parity.Deliverables checklist (ADR-0108)
docs/research/1099-sycl-fsycl-link-propagation.md## Alternatives consideredAGENTS.mdinvariant note:core/src/sycl/AGENTS.mdmeson test -C build --suite=fast test_sycl_motion_add_uv_parityon SYCL-capable hostchangelog.d/fixed/1099-sycl-fsycl-link-propagation.mddocs/rebase-notes.md(rebase-sensitive:core/src/meson.build,core/test/meson.build)State tracking (ADR-0165)
T-SYCL-MOTION-ADD-UV-SIGSEGV-2026-06-07moved from Open to Recently closed indocs/state.mdTest plan
meson test -C build-sycl --suite=fast test_sycl_motion_add_uv_parityon Intel Arc A380 host — expect PASS (not SIGSEGV)meson test -C build-sycl --suite=fast— all other SYCL parity tests still passmeson setup build -Denable_sycl=false && ninja -C build && meson test -C build --suite=fast— no regressions🤖 Generated with Claude Code