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

Hotfix : Update CMakeLists.txt to link libpthread #1290

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

rkamd
Copy link
Contributor

@rkamd rkamd commented Feb 2, 2023

Update CMakeLists.txt to link libpthread for all rocblas samples programs to avoid build error.

Update CMakeLists.txt for sample programs to link libpthread
Copy link
Contributor

@TorreZuk TorreZuk left a comment

Choose a reason for hiding this comment

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

Hopefully the guard isn't required as Threads::Threads target didn't exist with some old OS. Fine for now but would also tend to put more general library last in the list, i.e. rocblas before Threads::Threads.

Copy link
Contributor

@cgmb cgmb left a comment

Choose a reason for hiding this comment

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

This doesn't feel right to me. The error that prompted this was:

ld.lld: error: undefined reference due to --no-allow-shlib-undefined: pthread_create
>>> referenced by ../../library/src/librocblas.so.0.1.50500
clang-16: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [clients/samples/CMakeFiles/rocblas-example-c-dgeam.dir/build.make:100: clients/staging/rocblas-example-c-dgeam] Error 1

The error is saying that librocblas depends on pthreads. Yes, linking all the samples against pthreads will prevent the samples from failing to link, but my interpretation of this error is that other programs written by rocblas users will be seeing the same problem as the samples are. This is a problem in the rocblas library, not a problem in the samples.

If you look at library/src/CMakeLists.txt, you can see that the rocBLAS library build code does:

set(THREADS_PREFER_PTHREAD_FLAG ON)
find_package(Threads REQUIRED)

but it never actually uses the package that it finds. I believe that if the rocblas library itself was linked to pthreads, the usage requirement would be propagated to the samples automatically.

@TorreZuk
Copy link
Contributor

TorreZuk commented Feb 2, 2023

Ruban can you try what @cgmb suggested, as it may have gotten lost sometime in the past as we were getting the -lpthreads for free from hipcc. Just add it to the library PRIVATE Threads::Threads as we use it for logging so it may as well be added now. Not sure what you saw from ldd for it in your test but the library must have linked against it from some dependency?

@rkamd rkamd requested a review from cgmb February 3, 2023 00:05
@rkamd
Copy link
Contributor Author

rkamd commented Feb 3, 2023

@cgmb , Linked rocblas library itself to pthreads library.

Copy link
Contributor

@cgmb cgmb left a comment

Choose a reason for hiding this comment

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

Thanks. Assuming you've tested this and verified that it prevents the build failure, I think that is a better solution.

@rkamd
Copy link
Contributor Author

rkamd commented Feb 3, 2023

Thanks. Assuming you've tested this and verified that it prevents the build failure, I think that is a better solution.

Yes, this resolves build error.

@rkamd rkamd merged commit ed2a97f into release/rocm-rel-5.5 Feb 3, 2023
yoichiyoshida pushed a commit to yoichiyoshida/rocBLAS that referenced this pull request May 10, 2023
Updating to ROCm/Tensile@d22bb8e

Co-authored-by: ROCmMathLibrariesBot <no-reply@amd.com>
@amcamd amcamd deleted the swdev-380665-link-libpthread branch July 14, 2023 22:20
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.

None yet

4 participants