Skip to content

[SYCL] Return some Windows symbols in library back #4244

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

Conversation

dm-vodopyanov
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov commented Aug 3, 2021

This patch returns some of the symbols of the library which
had @oneapi@sycl@ before #4014 , and after that they were
renamed to @oneapi@ext@sycl@. These symbols can't be used
by users because the fully internal class stream_impl was exported,
so nothing breaks. Anyway, some symbols were removed in
comparison with Gold, and we can't change ABI yet. During the
timeframe where we can break ABI, we can fix stream_impl by
removing __SYCL_EXPORT from it.

This patch returns some unnecessary exported symbols on Windows which
could not be used by customers, and their absence didn't break anything
in user's application.
@dm-vodopyanov
Copy link
Contributor Author

/summary:run

@dm-vodopyanov dm-vodopyanov changed the title [SYCL] Return some removed unnecessary win symbols [SYCL] Return some unnecessary windows symbols Aug 3, 2021
@dm-vodopyanov
Copy link
Contributor Author

/summary:run

@dm-vodopyanov dm-vodopyanov marked this pull request as ready for review August 4, 2021 18:47
@dm-vodopyanov dm-vodopyanov requested a review from a team as a code owner August 4, 2021 18:47
@dm-vodopyanov dm-vodopyanov requested a review from rbegam August 4, 2021 18:47
@dm-vodopyanov
Copy link
Contributor Author

Jenkins/Summary failures cannot be related to this patch as test suite in it failed on Linux only. This patch has only Windows changes.
@bader if everything is ok, this PR can be merged.

@bader
Copy link
Contributor

bader commented Aug 5, 2021

Jenkins/Summary failures cannot be related to this patch as test suite in it failed on Linux only. This patch has only Windows changes.
@bader if everything is ok, this PR can be merged.

I would prefer if @romanovvlad gives another look and merge it. @dm-vodopyanov, please, file a ticket on unrelated failure.

@bader
Copy link
Contributor

bader commented Aug 5, 2021

This patch returns some unnecessary exported symbols on Windows which
could not be used by customers, and their absence didn't break anything
in user's application.

This sounds like you are adding some garbage to the library. Could you clarify why you are doing that?

@dm-vodopyanov
Copy link
Contributor Author

please, file a ticket on unrelated failure.

Created an internal ticket. The failure looks like an infra issue.

This sounds like you are adding some garbage to the library. Could you clarify why you are doing that?

That's absolutely true. These garbage symbols were removed from the library because Windows has very broad names of exported symbols and some of the symbols of the library had @ONEAPI@sycl@ before #4014 , and after that they were renamed to @oneapi@ext@sycl@. These symbols can't be used by users because the fully internal class stream_impl was exported, so nothing breaks. Anyway, the amount of symbols changed since Gold, and we can't change it yet. During timeframe where we can break ABI, we can fix stream_impl by removing __SYCL_EXPORT from it.

@dm-vodopyanov dm-vodopyanov changed the title [SYCL] Return some unnecessary windows symbols [SYCL] Return some Windows symbols in library back Aug 6, 2021
@bader bader merged commit 248da2e into intel:sycl Aug 9, 2021
@dm-vodopyanov dm-vodopyanov deleted the private/dvodopya/return-unnecessary-export-symbols-for-win branch February 10, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants