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 builds of UR fail due to undefined reference to std::filesystem #451

Closed
bmyates opened this issue Apr 18, 2023 · 9 comments · Fixed by #456
Closed

SYCL builds of UR fail due to undefined reference to std::filesystem #451

bmyates opened this issue Apr 18, 2023 · 9 comments · Fixed by #456
Assignees
Milestone

Comments

@bmyates
Copy link
Contributor

bmyates commented Apr 18, 2023

After addition in #439 some SYCL builds of UR are failing with undefined reference to std::filesystem::__cxx11::path::_M_split_cmpts()`

See this comment:: intel/llvm#9053 (comment)

@kbenzie
Copy link
Contributor

kbenzie commented Apr 19, 2023

@bmyates I'd ideally like to replicate this build environment in our CI so we can catch these issues before they reach SYCL, is it documented somewhere?

@pbalcer
Copy link
Contributor

pbalcer commented Apr 19, 2023

We could rip out the use of std::filesystem and replace it with two platform-specific implementations. It's not used for much. But I'm not sure if that's a good solution. UR's README states that it requires C++17, and that also means a C++ standard library that adequately implements its features. If we can't rely on that, and we do truly need to support platforms with old compilers, I'd prefer we revert to C++14 instead of playing endless whack-a-mole to support compilers/standard libraries with bugs or partial implementations.

It's not reasonable to expect that UR (or any project) is bug-compatible with every compiler and standard library version. We need to have stable target platforms (e.g., whatever ships in some stable versions of RHEL, SUSE, and Debian).
How does SYCL approach this? The getting started guide only lists GCC version 7.1.0 or later.

Also, I found this, which says that std::filesystem on GCC 8 requires adding -lstdc++fs. I think this should solve the immediate problem.

@kbenzie
Copy link
Contributor

kbenzie commented Apr 19, 2023

The C++17 requirement is inherited from upstream LLVM, I still think its reasonable for UR to also require it. I'd like to avoid reverting to C++14 if possible as <filesystem> is not the only instance of C++17 already in use.

This issue is frustrating though and I'd like to avoid getting bug reports from CI pipelines I'm not aware of moving forwards.

It's not reasonable to expect that UR (or any project) is bug-compatible with every compiler and standard library version. We need to have stable target platforms (e.g., whatever ships in some stable versions of RHEL, SUSE, and Debian).
How does SYCL approach this? The getting started guide only lists GCC version 7.1.0 or later.

Totally agree that we need to have a list of supported platforms, those at least need to line up with the ones in DPC++. Once we have that we can expand our CI and acceptance criteria for PRs to avoid recurrances moving forwards.

Also, I found this, which says that std::filesystem on GCC 8 requires adding -lstdc++fs. I think this should solve the immediate problem.

These lines in the root CMakeLists.txt should be setting -lstdc++fs globally for all UR targets but the link step for ur_loader doesn't have this option. I'll look into this.

@kbenzie kbenzie self-assigned this Apr 19, 2023
@bmyates
Copy link
Contributor Author

bmyates commented Apr 19, 2023

This issue is frustrating though and I'd like to avoid getting bug reports from CI pipelines I'm not aware of moving forwards.

I agree, it is frustrating for me as well. I have requested a list of required platforms from SYCL but have not received an answer yet.

@andykaylor
Copy link

I ran into this problem trying to build the SYCL compiler from the intel/llvm repository. I am using a RHEL8 system with gcc 8.3.1. It doesn't look like it's caused by a lack of C++17

@kbenzie
Copy link
Contributor

kbenzie commented Apr 21, 2023

RHEL 8 is be based on Fedora 28 and I've managed to reproduce this error there, working on a fix.

kbenzie added a commit to kbenzie/unified-runtime that referenced this issue Apr 21, 2023
This patch replaces changes the logic used to specify linking against
`libstdc++fs.a` required to use early iterations of `std::filesystem`.
Previously CMake checked for the existence of the `<filesystem>` header
to enable linking the static library, however in GCC 8 this header
exists while still requiring manual linking against the static library.
Now we check if we are compiling with `libstdc++` and always manually
link against the static library. Fixes oneapi-src#451.
kbenzie added a commit to kbenzie/unified-runtime that referenced this issue Apr 21, 2023
This patch replaces changes the logic used to specify linking against
`libstdc++fs.a` required to use early iterations of `std::filesystem`.
Previously CMake checked for the existence of the `<filesystem>` header
to enable linking the static library, however in GCC 8 this header
exists while still requiring manual linking against the static library.
Now we check if we are compiling with `libstdc++` and always manually
link against the static library. Fixes oneapi-src#451.
@kbenzie
Copy link
Contributor

kbenzie commented Apr 21, 2023

@bmyates / @andykaylor could you please verify the fix in #456 works as expected on the fialing RHEL builds?

@andykaylor
Copy link

@kbenzie #456 does fix the problem for me. Thanks!

@kbenzie
Copy link
Contributor

kbenzie commented Apr 24, 2023

Thanks @andykaylor, I'll merged the now.

I've created #457 to cover adding CI to try and avoid this reoccuring in future.

kbenzie added a commit that referenced this issue Apr 24, 2023
This patch replaces changes the logic used to specify linking against
`libstdc++fs.a` required to use early iterations of `std::filesystem`.
Previously CMake checked for the existence of the `<filesystem>` header
to enable linking the static library, however in GCC 8 this header
exists while still requiring manual linking against the static library.
Now we check if we are compiling with `libstdc++` and always manually
link against the static library. Fixes #451.
@kbenzie kbenzie added this to the 0.7 milestone Aug 3, 2023
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 a pull request may close this issue.

4 participants