Skip to content

[Clang][SYCL] Fix dangling pointers and ODR violation in free functions #20422

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 31, 2025
Merged

[Clang][SYCL] Fix dangling pointers and ODR violation in free functions #20422

merged 4 commits into from
Oct 31, 2025

Conversation

@uditagarwal97
Copy link
Contributor

@uditagarwal97 uditagarwal97 commented Oct 21, 2025 •
edited
Loading

Problems
Problem 1: When a library consisting of free function kernels is registered with SYCL RT, we store pointers (as string_view) to free function names in m_FreeFunctionKernelGlobalInfo but we do not remove them from m_FreeFunctionKernelGlobalInfo when the library is unloaded. Thus, we end up holding dangling pointers and any further operation on m_FreeFunctionKernelGlobalInfo might segfault.

Problem 2: Consider the case when you have multiple TUs with free functions and they are compiled separately but linked together into a single shared lib. In that case, we will have multiple definition of static GlobalMapUpdater updater in the shared lib => violating ODR

Solution
Discard pointers to free function names when library is unloaded and have GlobalMapUpdater defined in anonymous namespace, instead of sycl::v1::detail

@uditagarwal97 uditagarwal97 self-assigned this Oct 21, 2025
@uditagarwal97 uditagarwal97 changed the title [Clang][SYCL] Discard pointers to free function descriptors when library is unloaded [Clang][SYCL] Discard pointers to free function names when library is unloaded Oct 21, 2025
aelovikov-intel
aelovikov-intel reviewed Oct 21, 2025
View reviewed changes
@uditagarwal97 uditagarwal97 mentioned this pull request Oct 21, 2025
Merged
uditagarwal97 added a commit that referenced this pull request Oct 22, 2025
@uditagarwal97
[SYCL][test] Throw if order of ABI symbols is incorrect (#20424)
2f7b054 
Recently, there were several occurrences where developers manually
updated the ABI symbol dump file to add a new symbol. This results into
unrelated changes on PRs when someone correctly uses `abi_check.py` file
to regenerate the ABI symbols, (for example:
#20422)
This PR makes `abi_check.py` to fail if the order of ABI symbols is
incorrect, thus discouraging developers from manually editing the symbol
dump file.
@uditagarwal97 uditagarwal97 marked this pull request as ready for review October 22, 2025 14:59
@uditagarwal97 uditagarwal97 requested review from a team as code owners October 22, 2025 14:59
@uditagarwal97 uditagarwal97 force-pushed the private/udit/free_fun_perflib branch from fdaeb84 to eab57bd Compare October 22, 2025 15:10
@cperkinsintel
Copy link
Contributor

Should this have an E2E test? Though it's possible that this is already exercised by some of the IntermediateLib tests ( https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/IntermediateLib/multi_lib_app.cpp )

@uditagarwal97 uditagarwal97 force-pushed the private/udit/free_fun_perflib branch from 72823b4 to 2079eb2 Compare October 30, 2025 18:04
@uditagarwal97
Copy link
Contributor Author

Should this have an E2E test? Though it's possible that this is already exercised by some of the IntermediateLib tests ( https://github.com/intel/llvm/blob/sycl/sycl/test-e2e/IntermediateLib/multi_lib_app.cpp )

@cperkinsintel I'm finding it hard to reproduce the dangling pointer bug using an E2E/LIT test, although it's easily reproducible using valgrind. The rest of the code changes in this PR can be tested by existing FreeFunction tests.

@uditagarwal97
Copy link
Contributor Author

uditagarwal97 commented Oct 30, 2025 •
edited
Loading

Along with dangling pointers, there's another bug in current FreeFunction implementation.
It's due to violation of ODR because of multiple definitions of static GlobalMapUpdater updater, which can happen when you have multiple TU with free functions and they are compiled separately but linked together into a single shared lib.
To fix this bug, in 2079eb2 commit, I have moved GlobalMapUpdater into an anonymous namespace instead of sycl::v1::detail.

@uditagarwal97 uditagarwal97 changed the title [Clang][SYCL] Discard pointers to free function names when library is unloaded [Clang][SYCL] Fix dangling pointers and ODR violation in free functions Oct 30, 2025
@uditagarwal97
Copy link
Contributor Author

@elizabethandrews (@intel/dpcpp-cfe-reviewers) Can you please re-review the FE changes? I made a minor change (2079eb2) since the last review. Thanks!

@github-actions
Copy link
Contributor

@intel/llvm-gatekeepers please consider merging

premanandrao
premanandrao reviewed Oct 31, 2025
View reviewed changes
Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor nits.

clang/test/SemaSYCL/Inputs/sycl/detail/kernel_global_info.hpp
namespace free_function_info_map {

__SYCL_EXPORT void add(const void *DeviceGlobalPtr, const char *UniqueId);
__SYCL_EXPORT void remove(const void *DeviceGlobalPtr, const char *UniqueId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were the changes in this file needed? (I understand it is the mock header, but I don't see this tested in SemaSYCL anywhere.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dklochkov-emb since this file was added in https://github.com/intel/llvm/pull/19517/files#diff-dfac134a49adec0ea0525b0941ce5b5e8b51c64e0692aae6c8f0105b9936106e, do you know how is it being used?

Copy link
Contributor

@dklochkov-emb dklochkov-emb Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I know it is needed for SemaSYCL tests, i.e. it mocks real header. For example, test file
https://github.com/intel/llvm/blob/sycl/clang/test/SemaSYCL/free_function_kernel_params.cpp

contains -internal-isystem %S/Inputs. In my understanding, this file will be visible to include as the system file after that

@uditagarwal97 uditagarwal97 merged commit 6b4e685 into sycl Oct 31, 2025
28 checks passed
@uditagarwal97 uditagarwal97 deleted the private/udit/free_fun_perflib branch October 31, 2025 16:28
@dklochkov-emb dklochkov-emb mentioned this pull request Nov 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dklochkov-emb dklochkov-emb dklochkov-emb approved these changes

@cperkinsintel cperkinsintel cperkinsintel approved these changes

@elizabethandrews elizabethandrews elizabethandrews approved these changes

@steffenlarsen steffenlarsen Awaiting requested review from steffenlarsen

@aelovikov-intel aelovikov-intel Awaiting requested review from aelovikov-intel

@premanandrao premanandrao Awaiting requested review from premanandrao

Assignees

@uditagarwal97 uditagarwal97

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@uditagarwal97 @cperkinsintel @premanandrao @elizabethandrews @aelovikov-intel @dklochkov-emb