OpenBLAS icon indicating copy to clipboard operation
OpenBLAS copied to clipboard

[WIP] Set visibility attribute on internal function symbols to hidden

Open martin-frbg opened this issue 3 years ago • 3 comments

cf. https://github.com/MacPython/openblas-libs/issues/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)

martin-frbg avatar Jun 19 '22 12:06 martin-frbg

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.

mmuetzel avatar Jul 06 '22 12:07 mmuetzel

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 ?

martin-frbg avatar Jul 06 '22 12:07 martin-frbg

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.

mmuetzel avatar Jul 06 '22 13:07 mmuetzel

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 https://github.com/scipy/scipy/pull/11193 talks about how use of 32-bit and 64-bit BLAS libraries at the same time is done.

rgommers avatar Jun 13 '23 19:06 rgommers