Skip to content

Conversation

@martin-frbg
Copy link
Collaborator

cf. MacPython/openblas-libs#79 (remaining symbol conflicts between builds with and without INTERFACE64 as the internally used functions to not get suffixed. Adding suffixes for them would be the second-best solution if some compiler balks at the attribute)

@mmuetzel
Copy link
Contributor

mmuetzel commented Jul 6, 2022

It would be nice if this could be resolved in a way that works cross-platform.

Unfortunately, Windows doesn't understand __attribute__((visibility("hidden"))). Instead, it uses a dllexport-dllimport mechanism. Essentially, if the linker detects that some symbols are marked for dllexport it only exports those symbols (and hides all the other ones). To make that kind of compatible with POSIX, all symbols that should not be hidden could be tagged with __attribute__((visibility("default"))). And -fvisibility=hidden could be added to the compiler flags. (So, basically the other way round to what is done in this PR.)

See also, e.g., https://www.gnu.org/software/gnulib/manual/html_node/Exported-Symbols-of-Shared-Libraries.html for a more complete description.

@martin-frbg
Copy link
Collaborator Author

The PR so far has the attribute setting conditional on C_MSVC not being set, and my understanding from the original issue is that no similar problem was observed in their Windows builds. Are you suggesting that this approach is wrong ?

@mmuetzel
Copy link
Contributor

mmuetzel commented Jul 6, 2022

The approach here isn't wrong. It's just not easily portable to Windows (because on that platform, there is just a flag for "export this" and there is none for "don't export this").

I'm not sure what the "dll namespaces" could be that are supposedly helping on Windows.
I haven't actually seen a project that links to both the 64-bit and 32-bit Fortran integer version of OpenBLAS at the same time. But if there are symbols with the same name in both versions, they would also clash on Windows iiuc. (Or I didn't understand the original issue.)

The instructions at the end of the Gnulib article miss the correct flags for MinGW. Something like the following in a header that is included in all files that export symbols could work:

#if defined (_WIN32) || defined (__CYGWIN__)
#  if defined (__GNUC__)
     /* GCC */
#    define OPENBLAS_EXPORT __attribute__ ((dllexport))
#    define OPENBLAS_IMPORT __attribute__ ((dllimport))
#  else
     /* MSVC */
#    define OPENBLAS_EXPORT __declspec(dllexport)
#    define OPENBLAS_IMPORT __declspec(dllimport)
#  endif
#else
   /* All other platforms. */
#  define OPENBLAS_EXPORT __attribute__ ((visibility ("default")))
#  define OPENBLAS_IMPORT
#endif

#if defined (OPENBLAS_SHLIB)
#  define OPENBLAS_API OPENBLAS_EXPORT 
#else
#  define OPENBLAS_API OPENBLAS_IMPORT
#endif

With that, all symbols that should be exported could be tagged with OPENBLAS_API.
During compilation of the shared OpenBLAS library, -DOPENBLAS_SHLIB could be passed as a compiler flag (on all platforms). When linking to the OpenBLAS library from another project that flag shouldn't be defined.
On platforms that support it (all but Windows?), -fvisibility=hidden could be added to the compiler flags.

@rgommers
Copy link
Contributor

I haven't actually seen a project that links to both the 64-bit and 32-bit Fortran integer version of OpenBLAS at the same time.

I'll note that SciPy does this; it has many separate submodules with one or multiple Python extension modules using BLAS/LAPACK, and the support for ILP64 was implemented in stages (and is still incomplete). The PR description at scipy/scipy#11193 talks about how use of 32-bit and 64-bit BLAS libraries at the same time is done.

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.

3 participants