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

Compile all arbor source files with -fvisibility=hidden #1599

Merged
merged 5 commits into from
Jul 2, 2021

Conversation

noraabiakar
Copy link
Contributor

@noraabiakar noraabiakar commented Jun 25, 2021

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 with fvisibility=hidden.

Fixes #1557
Fixes #1559

@noraabiakar noraabiakar marked this pull request as draft June 25, 2021 15:06
@noraabiakar noraabiakar changed the title Compile all arbor source files with -fvisibility=hidden 🚧 Compile all arbor source files with -fvisibility=hidden Jun 25, 2021
@noraabiakar noraabiakar changed the title 🚧 Compile all arbor source files with -fvisibility=hidden [WIP] Compile all arbor source files with -fvisibility=hidden Jun 25, 2021
@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 25, 2021
@bors
Copy link

bors bot commented Jun 25, 2021

try

Build failed:

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 28, 2021
@bors
Copy link

bors bot commented Jun 28, 2021

try

Build failed:

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 28, 2021
@bors
Copy link

bors bot commented Jun 28, 2021

try

Timed out.

@noraabiakar
Copy link
Contributor Author

bors try

bors bot added a commit that referenced this pull request Jun 28, 2021
@bors
Copy link

bors bot commented Jun 28, 2021

try

Build succeeded:

@noraabiakar noraabiakar marked this pull request as ready for review June 28, 2021 13:56
@noraabiakar noraabiakar changed the title [WIP] Compile all arbor source files with -fvisibility=hidden Compile all arbor source files with -fvisibility=hidden Jun 28, 2021
@thorstenhater thorstenhater self-assigned this Jun 28, 2021
@kanzl
Copy link
Contributor

kanzl commented Jun 28, 2021

Works for me on M1 Macbook

Copy link
Contributor

@thorstenhater thorstenhater left a 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.

@@ -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() {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

@espenhgn
Copy link
Collaborator

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!

@noraabiakar noraabiakar merged commit 5c5fd64 into arbor-sim:master Jul 2, 2021
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.

RuntimeError: Error parsing the label: in python functions and example files
4 participants