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

[loader] Add use of threads so -lpthread works when building #797

Merged
merged 6 commits into from
Aug 16, 2023

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Aug 11, 2023

This patch makes libur_loader.so force link against pthreads by adding some trivial use of threads. Linking against pthreads is necessary for std::call_once to work for gcc 9.4, but it doesn't happen automatically, as even when specifying -lpthread, the linker doesn't know that pthreads are necessary for call_once (pthreads isn't necessary for call_once for other versions of gcc) so it may elide the linking.

@hdelan
Copy link
Contributor Author

hdelan commented Aug 11, 2023

I am using gcc-9.4.0 and in this version the definition of std::call_once requires the binary to be linked with pthreads. If it isn't linked with pthreads it will still compile OK and error out when it is called:

 $ g++ call_once.cpp
$ ./a.out
terminate called after throwing an instance of 'std::system_error'
  what():  Unknown error -1
[1]    184552 abort (core dumped)  ./a.out

This bug is documented here https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55394

So in the cmake for ur_adapter, Threads are required. But this translates to -lpthread when linking. The linker then seems to do some scanning of the module, and if it only sees call_once and no other mentions of threads, it will not link to pthreads. (presumably because other versions of gcc don't require linking with pthreads in order for call_once to work)(Aside: a heavier version of -lpthread is -pthread which will force link with pthreads as well as define some macros etc and make it work)

$ g++ call_once_no_threads.cpp -lpthread
$ ldd a.out
    linux-vdso.so.1 (0x00007fff2b147000)
    libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f44e9a1a000)
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f44e9828000)
    libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f44e96d9000)
    /lib64/ld-linux-x86-64.so.2 (0x00007f44e9c24000)
    libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f44e96be000)
$ ./a.out
terminate called after throwing an instance of 'std::system_error'
  what():  Unknown error -1
[1]    184552 abort (core dumped)  ./a.out

Whereas if we have some use of threads we get:

$ g++ call_once_with_a_thread.cpp -lpthread
$ ldd a.out
    linux-vdso.so.1 (0x00007fffcb9fc000)
    libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007ff8f71ef000)
    libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007ff8f700d000)
    libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007ff8f6ff2000)
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007ff8f6e00000)
    /lib64/ld-linux-x86-64.so.2 (0x00007ff8f723d000)
    libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007ff8f6caf000)
$ ./a.out
Success!

So in my case the libur_adapter.so was not getting linked with pthreads which was causing an error out on call_once

$ ldd lib/libur_loader.so
    linux-vdso.so.1 (0x00007ffd0615f000)
    libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fdc9169c000)
    libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fdc914ba000)
    libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fdc9149f000)
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fdc912ad000)
    /lib64/ld-linux-x86-64.so.2 (0x00007fdc91720000)
    libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fdc9115c000)

With this change the new output is:

$ ldd lib/libur_loader.so
	linux-vdso.so.1 (0x00007ffc031af000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fc922aa6000)
	libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fc922aa0000)
	libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fc9228be000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fc9228a3000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fc9226af000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fc922b48000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fc922560000)

@hdelan
Copy link
Contributor Author

hdelan commented Aug 11, 2023

Ping @ldrumm

@callumfare
Copy link
Contributor

callumfare commented Aug 11, 2023

ur_libapi.cpp is auto-generated from a template (with the generate target), so the function would have to live in a different file like ur_api.cpp (or be added to the template like @veselypeta said)

One other solution to this would be conditionally adding -pthread to the loader in cmake if using a known bad version of gcc. I don't know if that's portable across everything that we target. Looking at the comment on the gcc issue, it was fixed by the call_once implementation changing in GLIBCXX_3.4.29 which corresponds to GCC 11.1.

@hdelan
Copy link
Contributor Author

hdelan commented Aug 11, 2023

Aha silly me thanks @veselypeta @callumfare

Copy link
Contributor

@veselypeta veselypeta left a comment

Choose a reason for hiding this comment

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

LGMT, just one suggestion 👍

source/loader/ur_libapi.cpp Outdated Show resolved Hide resolved
@kbenzie
Copy link
Contributor

kbenzie commented Aug 15, 2023

One other solution to this would be conditionally adding -pthread to the loader in cmake if using a known bad version of gcc. I don't know if that's portable across everything that we target. Looking at the comment on the gcc issue, it was fixed by the call_once implementation changing in GLIBCXX_3.4.29 which corresponds to GCC 11.1.

I think this would be a preferable workaround to me.

@hdelan
Copy link
Contributor Author

hdelan commented Aug 15, 2023

I think this would be a preferable workaround to me.

Change made

@hdelan
Copy link
Contributor Author

hdelan commented Aug 16, 2023

@kbenzie can we merge this?

@ldrumm
Copy link
Contributor

ldrumm commented Aug 16, 2023

Looks good. The cmake approach is the right one, and it makes it clear that this the extra flag is a workaround for a specific compiler, so it can be removed later

Definitely a fun bug to track down!

@hdelan
Copy link
Contributor Author

hdelan commented Aug 16, 2023

Can someone with merge privileges merge this please?

@kbenzie kbenzie merged commit 2970796 into oneapi-src:main Aug 16, 2023
24 checks passed
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.

5 participants