Skip to content
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

Add loader API map/def files #1664

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Conversation

PatKamin
Copy link
Contributor

Export only the loader API symbols that are needed. This is done by creating a map file for the loader API for Linux and a def file for Windows.

This is to fix the issue of having the same set of symbols exported both by libur_loader and libigc leading to UndefinedBehavior. This issue occurs for UR binaries built with ASan and DisableDeepBind=1 compute-runtime env var set while running a binary.

@PatKamin PatKamin requested a review from a team as a code owner May 24, 2024 12:09
@PatKamin PatKamin requested a review from pbalcer May 24, 2024 12:10
@github-actions github-actions bot added the loader Loader related feature/bug label May 24, 2024
Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

I could've sworn this already existed :)

source/loader/CMakeLists.txt Outdated Show resolved Hide resolved
scripts/templates/loader.map.in.mako Outdated Show resolved Hide resolved
@PatKamin PatKamin force-pushed the loader-map-files branch 2 times, most recently from fc664e8 to 00cb3ae Compare May 27, 2024 10:06
@pbalcer
Copy link
Contributor

pbalcer commented May 27, 2024

/e2e-level-zero

Copy link

Copy link

E2E L0 build:
https://github.com/oneapi-src/unified-runtime/actions/runs/9254960977
Job status: success. Test status: success

Copy link
Contributor

@kbenzie kbenzie left a comment

Choose a reason for hiding this comment

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

A new entry point was added, the latest main branch changes need pulled in and then run the generate target.

set_target_properties(ur_loader PROPERTIES
LINK_FLAGS "/DEF:${LOADER_VERSION_SCRIPT}"
)
else()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be checking if its Linux:

Suggested change
else()
elseif(CMAKE_SYSTEM_NAME STREQUAL Linux)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done.

Export only the loader API symbols that are needed. This is done
by creating a map file for the loader API for Linux and a def file for
Windows.
@PatKamin
Copy link
Contributor Author

PatKamin commented Jun 6, 2024

A new entry point was added, the latest main branch changes need pulled in and then run the generate target.

Updated

@kbenzie
Copy link
Contributor

kbenzie commented Jun 6, 2024

The failure is a known issue #1715, merging.

@kbenzie kbenzie merged commit a52eac5 into oneapi-src:main Jun 6, 2024
53 of 54 checks passed
@PatKamin PatKamin deleted the loader-map-files branch June 26, 2024 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loader Loader related feature/bug ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants