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

[SYCL][TEST] Test to check ABI neutrality of SYCL RT #14492

Merged
merged 14 commits into from
Jul 12, 2024

Conversation

bso-intel
Copy link
Contributor

No description provided.

Co-authored-by: Steffen Larsen <steffen.larsen@intel.com>
@againull againull marked this pull request as ready for review July 11, 2024 19:35
@againull againull requested a review from a team as a code owner July 11, 2024 19:35
@againull againull self-requested a review July 11, 2024 19:35
@againull againull marked this pull request as draft July 11, 2024 19:41
@againull againull marked this pull request as ready for review July 11, 2024 19:42
@againull againull changed the title [SYCL][TEST] Test for pre-C++11 symbols in dump [SYCL][TEST] Test to check ABI neutrality of SYCL RT Jul 12, 2024
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

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

LGTM, just some non-blocking nitpicks on the wording.

// UNSUPPORTED: libcxx
// RUN: FileCheck %s --input-file %S/sycl_symbols_linux.dump

// The purpose of this test is to check that all symbols which are visible from
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The purpose of this test is to check that all symbols which are visible from
// The purpose of this test is to check that all symbols that are visible from

// with "cxx11" tag because such symbols correspond to the new ABI entries
// (_GLIBCXX_USE_CXX11_ABI=1, default) and won't work with a program that uses
// the old ABI (_GLIBCXX_USE_CXX11_ABI=0). All APIs exported from SYCL RT must
// avoid using classes like std::string and std::list impacted by dual-abi issue
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// avoid using classes like std::string and std::list impacted by dual-abi issue
// avoid using classes impacted by the dual ABI issue like std::string and std::list

// (_GLIBCXX_USE_CXX11_ABI=1, default) and won't work with a program that uses
// the old ABI (_GLIBCXX_USE_CXX11_ABI=0). All APIs exported from SYCL RT must
// avoid using classes like std::string and std::list impacted by dual-abi issue
// and have to use abi-neutral counterpart provided by SYCL RT (e.g
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// and have to use abi-neutral counterpart provided by SYCL RT (e.g
// and have to use their ABI-neutral counterparts provided by SYCL RT (e.g

// and have to use abi-neutral counterpart provided by SYCL RT (e.g
// sycl::detail::string, etc.).

// New exclusions are NOT ALLOWED to this file. All remaining cases which need
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// New exclusions are NOT ALLOWED to this file. All remaining cases which need
// New exclusions are NOT ALLOWED to this file. All remaining cases that need

@againull againull merged commit 36543a1 into intel:sycl Jul 12, 2024
10 of 12 checks passed
@bso-intel bso-intel deleted the string-check branch July 24, 2024 18:02
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