[SYCL][Unittests] Don't pass addresses of local variables as UR handles in unit tests - #23105
Open
uditagarwal97 wants to merge 1 commit into
Open
Conversation
…nit tests Three unit tests handed the address of a stack variable to the runtime as a ur_event_handle_t / native cl_kernel. The mock adapter treats every handle as a mock::dummy_handle_t_ and increments or decrements its reference counter, so the runtime's release calls were writing into the tests' stack frames. Use real dummy handles instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes correctly eliminate invalid stack-backed handles and balance mock handle ownership.
Pull request overview
Fixes unsafe mock-handle ownership in three SYCL unit tests.
Changes:
- Replaces stack addresses with valid reference-counted dummy handles.
- Ensures interop kernels are destroyed before releasing test-owned handles.
File summaries
| File | Description |
|---|---|
sycl/unittests/thread_safety/InteropKernelEnqueue.cpp |
Corrects interop kernel handle lifetime. |
sycl/unittests/handler/SetArgForLocalAccessor.cpp |
Corrects interop kernel handle lifetime. |
sycl/unittests/Extensions/BindlessImages/Semaphores.cpp |
Uses releasable dummy event handles. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Problem
The UR mock adapter reinterpret_casts every handle it receives to
mock::dummy_handle_t_and touchesMRefCounteron retain/release. Three unit tests passed the address of a local variable where the runtime expects a handle, so those retain/release calls wrote into the tests' own stack frames.1.
sycl/unittests/Extensions/BindlessImages/Semaphores.cpp—BindlessImagesExtensionTests.ExternalSemaphoreSignalbuilt fake device events with~event_impl()callsurEventReleaseon those handles:The same pattern also produced a UBSan
member access within misaligned address ... for type 'dummy_handle_t_'diagnostic, because a 4-byte-alignedintis not suitably aligned for the handle struct.2.
sycl/unittests/thread_safety/InteropKernelEnqueue.cpp(KernelEnqueue.InteropKernel) and 3.sycl/unittests/handler/SetArgForLocalAccessor.cpp(HandlerSetArg.LocalAccessor) — both created a native handle and then passed its address tomake_kernel:so the runtime's retain/release of the interop kernel mutated the local
Handlevariable, and the handle it actually created was never released (a leak on top of the corruption).Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com