-
Notifications
You must be signed in to change notification settings - Fork 787
Export std hash template specializations from dylibs #7618
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why those specific places need the workaround? I'd expect more, if this was a general issue, but I could be wrong...
Yes, those are the particular specializations that are used from code that is not linked into the dylib, but is linked into the tools (i.e. a dependency from the tool object file to libbinaryen.so). Anything linked into libbinaryen.so doesn't have this problem. (and just in case it wasn't clear, this only affects functions declared in namespace std). |
Makes sense, though I am surprised that e.g. |
In any case, lgtm if this fixes the libc++ issue. |
@@ -462,6 +462,10 @@ if(BUILD_STATIC_LIB) | |||
else() | |||
message(STATUS "Building libbinaryen as shared library.") | |||
add_library(binaryen SHARED) | |||
if(LINUX) | |||
# Disable interposition and resolve Binaryen symbols locally. | |||
add_link_flag("-Bsymbolic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is not actually necessary for the workaround. It's just a small optimization for DSO-local calls.
I'm thinking I might land this for now to unblock the LLVM roll. I'm not enthusiastic about this as a long-term solution, but I think for now we can wait and see if there's any resolution to the upstream patch, and re-evaluate. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to land for now.
Use explicit visibility to force std::hash template specializations to be
exported from libbinaryen dynamic library.
Currently
we are just using the default flags, causing most of our symbols to have
"default" visibility, meaning they can all be used from the tool sources.
However a recent libc++ change caused functions declared in namespace std
(e.g. hash template specializations) to have hidden visibility, inherited.
from the namespece (see llvm/llvm-project#131156).
So this macro forces them to be exported. Currently it is only applied to
the hash specializations that are defined in libbinaryen and used in the
tool code. In the future if we want to compile libbinaryen with
-fvisibility-hidden or use a DLL on Windows, we'll need
to explicitly annotate everything we want to export. But that's probably
only useful if we want external users to link against libbinaryen.so
(currently we don't; it's only used to reduce our install size).