Skip to content

Conversation

@DanielCChen
Copy link
Contributor

This patch is to explicitly link the pthread library when building shared flang-rt.
On AIX, for example, it needs to link in libpthread.a explicitly in order to resolve the references to those pthread_* functions in include/flang-rt/runtime/lock.h

@DanielCChen DanielCChen self-assigned this Mar 5, 2025
endif ()
if (build_shared)
add_library("${name_shared}" SHARED ${extra_args} ${ARG_ADDITIONAL_HEADERS} ${ARG_UNPARSED_ARGUMENTS})
find_package(Threads REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

Can you move find_package(Threads REQUIRED) to flang-rt/CMakeLists.txt? That's where all the requirements are being collected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will do.

Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Copy link
Member

Choose a reason for hiding this comment

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

Would be great to comment about why pthread is needed/what for, instead of what it is doing.

https://www.jackfranklin.co.uk/blog/code-comments-why-not-how/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will update the comments.
Thanks for the pointer to the blog!

Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

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

LGTM (for new comment)

@DanielCChen
Copy link
Contributor Author

@Meinersbur
I have made another change to remove the REQUIRED and added a check to only link the pthread library if it exists because not all platform (e.g WIN32) has POSIX thread library.
Could you please take another look?

Copy link
Member

@Meinersbur Meinersbur left a comment

Choose a reason for hiding this comment

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

I expected it to be trivially present on Windows, as part of the Win32 CRT (there is a CMAKE_USE_WIN32_THREADS_INIT results variable).

Aynway, LGTM

@DanielCChen DanielCChen merged commit 78631ac into llvm:main Mar 7, 2025
9 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.

2 participants