-
Notifications
You must be signed in to change notification settings - Fork 62
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
Compile all arbor source files with -fvisibility=hidden
#1599
Conversation
-fvisibility=hidden
-fvisibility=hidden
-fvisibility=hidden
-fvisibility=hidden
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
bors try |
tryTimed out. |
bors try |
tryBuild succeeded: |
-fvisibility=hidden
-fvisibility=hidden
Works for me on M1 Macbook |
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.
Looks good, also works on my non-M1 MacBook. A tiny standards nitpick and we are good to merge.
mechanisms/generate_catalogue
Outdated
@@ -120,7 +120,7 @@ const mechanism_catalogue& global_${catalogue}_catalogue() { | |||
|
|||
#ifdef STANDALONE | |||
extern "C" { | |||
const void* get_catalogue() { | |||
__attribute__ ((visibility ("default"))) const void* get_catalogue() { |
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.
Since C++17 we are allowed to use [[gnu::visibility("default")]]
as part of the standard. The __attribute__
is technically an extension by GCC.
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.
But that would only work for GCC. According to this: https://gcc.gnu.org/wiki/Visibility, we should add this entire block:
#if defined _WIN32 || defined __CYGWIN__
#ifdef BUILDING_DLL
#ifdef __GNUC__
#define DLL_PUBLIC __attribute__ ((dllexport))
#else
#define DLL_PUBLIC __declspec(dllexport) // Note: actually gcc seems to also supports this syntax.
#endif
#else
#ifdef __GNUC__
#define DLL_PUBLIC __attribute__ ((dllimport))
#else
#define DLL_PUBLIC __declspec(dllimport) // Note: actually gcc seems to also supports this syntax.
#endif
#endif
#define DLL_LOCAL
#else
#if __GNUC__ >= 4
#define DLL_PUBLIC __attribute__ ((visibility ("default")))
#define DLL_LOCAL __attribute__ ((visibility ("hidden")))
#else
#define DLL_PUBLIC
#define DLL_LOCAL
#endif
#endif
I think we need that top block for WSL, but I'm not sure and I don't have a system to test it on.
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.
That page predates the C++17 attribute (last change 2016). We also can savely assume GCC > 4 (else C++14 would be tricky). And finally, we do not care about Windows right now. In endeffect it comes down to what you have in the PR.
Regarding your comment, this is working in clang and gcc. To quote my last comment __attribute__
is a GCC extension in itself. However, Clang shares most of GCC's extensions. Reference: https://gcc.godbolt.org/z/Edev9PGnK
Hi. As I raised #1557 I tested this PR out locally, and it appears to have sorted out the problem(s) I encountered there. Thanks! |
The visibility of symbols in the python shared library is set to 'hidden'. However, checking the dynamic symbol table of the generated
.so
file reveals that all the symbols of the static arbor libraries are still visible. This causes some issues on the Mac M1. To resolve, all source files should be compiled withfvisibility=hidden
.Fixes #1557
Fixes #1559