fix: Fix symbol export visibility #531
Conversation
| @@ -122,7 +122,7 @@ int main(int argc, char* argv[]) { | |||
|
|
|||
| end = clock(); | |||
| elapsed = (end - begin) / ((double)CLOCKS_PER_SEC); | |||
| fprintf(stdout, "Read %l" PRId64 " rows in %" PRId64 " batch(es) <%.06f seconds>\n", | |||
| fprintf(stdout, "Read %" PRId64 " rows in %" PRId64 " batch(es) <%.06f seconds>\n", | |||
There was a problem hiding this comment.
Unrelated but figure minor enough to toss in
| @@ -20,10 +20,20 @@ | |||
|
|
|||
| #include <stdint.h> | |||
|
|
|||
| #if defined(_MSC_VER) | |||
| #define DLL_EXPORT __declspec(dllexport) | |||
| #if defined _WIN32 || defined __CYGWIN__ | |||
There was a problem hiding this comment.
Maybe this should be put into a common header instead of here
There was a problem hiding this comment.
I think nanoarrow_types.h would be the right place!
There was a problem hiding this comment.
I think the C library still needs the attribute on Windows (but I can go through and do this in a later PR)
| @@ -20,10 +20,20 @@ | |||
|
|
|||
| #include <stdint.h> | |||
|
|
|||
| #if defined(_MSC_VER) | |||
| #define DLL_EXPORT __declspec(dllexport) | |||
| #if defined _WIN32 || defined __CYGWIN__ | |||
There was a problem hiding this comment.
I think nanoarrow_types.h would be the right place!
| @@ -76,19 +86,19 @@ struct ArrowArray { | |||
| #endif // ARROW_C_DATA_INTERFACE | |||
| #endif // ARROW_FLAG_DICTIONARY_ORDERED | |||
|
|
|||
| DLL_EXPORT const char* nanoarrow_CDataIntegration_ExportSchemaFromJson( | |||
| NANOARROW_DLL_EXPORT const char* nanoarrow_CDataIntegration_ExportSchemaFromJson( | |||
There was a problem hiding this comment.
I believe that all of the functions in nanoarrow.h, nanoarrow_device.h, and nanoarrow_ipc.h will need NANOARROW_DLL_EXPORT as well.
Another way to think about it would be every symbol listed in the namespace defines (also the ones listed in nanoarrow_device_xxx.h and nanoarrow_ipc.h):
arrow-nanoarrow/src/nanoarrow/nanoarrow.h
Lines 38 to 137 in 1febb81
There was a problem hiding this comment.
Sounds good - went ahead and added the visibility to the C compiler and used the macro for any of the defines.
There might be a few defines missing? I noticed things like ArrowSchemaMove, ArrowArrayMove and ArrowArrayStreamMove, etc... were not part of the defines but the IPC/device move functions always were
There was a problem hiding this comment.
To my understanding, any function that is static inline doesn't need to worry about this (i.e., those functions are header-only and are never public).
There was a problem hiding this comment.
Correcting myself here...they're header-only but some of them are public. (None of them end up as exported symbols, though)
src/nanoarrow/nanoarrow_types.h
Outdated
| #if defined _WIN32 || defined __CYGWIN__ | ||
| #define NANOARROW_DLL_EXPORT __declspec(dllexport) | ||
| #else | ||
| #if __GNUC__ >= 4 | ||
| #define NANOARROW_DLL_EXPORT __attribute__((visibility("default"))) | ||
| #else | ||
| #define NANOARROW_DLL_EXPORT | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
I think that using Abseil's strategy might be a good one here (only using Abseil as an example because I recently had to deal with linking it on Windows):
I am not an expert here, but I think that in a static linking scenario (the intended mechanism for consuming nanoarrow, whether this is via including nanorrow.c in sources or statically linking libnanoarrow.a), we need #define NANOARROW_DLL_EXPORT to be empty.
When building shared libraries on Windows, we can very specifically set -DNANOARROW_BUILD_DLL from Cmake or Meson; when consuming a shared nanoarrow, CMake or Meson can set -DNANOARROW_CONSUME_DLL (in CMake this might be via generator expressions but I'm not sure what it would look like in Meson other than perhaps that you could manually add it when building the tests).
So maybe:
#if defined(defined _WIN32 || defined __CYGWIN__)
#if defined(NANOARROW_BUILD_DLL)
#define NANOARROW_DLL __declspec(dllexport)
#elif defined(NANOARROW_CONSUME_DLL)
#define NANOARROW_DLL __declspec(dllimport)
#else
#define NANOARROW_DLL
#endif
#else
#define NANOARROW_DLL
#endifThere was a problem hiding this comment.
Ah thanks for the info. Also not an expert here so good to see these pitfalls.
Meson also has an argument for libraries called gnu_symbol_visibility that can be toggled. I'll ask on IRC about that and what it means for all the different build contexts. Generally I hope to trust Meson to just do the right thing
src/nanoarrow/nanoarrow_types.h
Outdated
| #if __GNUC__ >= 4 | ||
| #define NANOARROW_EXPORT __attribute__((visibility("default"))) | ||
| #else | ||
| #define NANOARROW_EXPORT | ||
| #endif // __GNUC__ >= 4 |
There was a problem hiding this comment.
I think that this can be omitted (or if it is added, added in another PR where its effect can get more specific review)!
As I understand it, if somebody is statically linking nanoarrow (which they probably are, or should be), we very much do not want the nanoarrow symbols to be visible, particularly if the outer project is trying to suppress those symbols via -fvisibility=hidden.
Many projects have a number of "internal" headers or functions that are defined in other compilation units; however, nanoarrow does not: all internal functions are marked static (i.e., not visible ever anywhere outside the same .c file), and all shared functions are either static inline (i.e., the function only exists in the header where it is either inlined when used or made into a static function at the compiler's option) or declared without static. As I understand it, this means that -fvisibility=default will do the right thing when building a libnanoarrow.so with gcc, and no attribute is needed.
There was a problem hiding this comment.
Which part of the macro are you thinking we should remove? Just the #define NANOARROW_EXPORT or the entire gcc block?
From what I understand, MSVC requires you to be cognizant of all the different contexts and explicitly ask for what you want. GCC manages this in a more automated fashion:
There was a problem hiding this comment.
The entire non-Windows block I believe should read #define NANOARROW_EXPORT (the Windows part of that is doing the right thing). (And probably this whole block should be wrapped in #if !defined(NANOARROW_EXPORT) to give users an escape hatch if our logic was wrong).
There was a problem hiding this comment.
If you set gcc universally to #define NANOARROW_EXPORT then linkage there will fail, since we are compiling these librarys with -fvisibility=hidden in the Meson configuration
There was a problem hiding this comment.
compiling these librarys with
-fvisibility=hidden
I believe that compiling with -fvisibility=hidden is not necessary (you could check the output of nm to see what the difference is).
In any case, the visibility attribute should be very explicitly added only when NANOARROW_BUILD_DLL is defined (the issue being that if nanoarrow is statically linked into somebody else's library, we don't want or need default symbol visibility because nanoarrow shouldn't be accessed from somebody else's shared library).
There was a problem hiding this comment.
I see from that comment that gcc may take care of the case where it is being statically linked into somebody else's something; however, I haven't seen that attribute used in this way before and I'm hesitant to include it without some evidence that it is helping in some way. (Very possible that my mental model here is off!)
Maybe a good way to put how we've structured the functions here is that there basically are no "private" functions (everything that is not static or static inline should be exported).
There was a problem hiding this comment.
I think a good way to test this would be to try the Meson subproject configuration, since that will statically link nanoarrow. Looks like that is broken on main at the moment so will get that fixed so we can more accurately compare
There definitely are symbols that are not getting exported properly (note the original commit in this PR, which was only scoped to C++); having the ability to detect these from gcc I think is helpful
There was a problem hiding this comment.
Got it! I'd forgotten that we build shared libraries with C++ sources (currently just the integration test, where symbol visibility doesn't really matter, but perhaps soon nanoarrow_testing and nanoarrow_device with Metal). Basically, we want symbols for header-only C++ things to not make it in the final .so (only the final C FFI).
Since the GCC attribute is currently just a C++ issue scoped to the integration test, that is probably the best place to put this (e.g., your first commit 😬 ) and we can sort it out for other places that need this later.
A reproducer including both C and C++:
(.venv) deweydunnington@Deweys-Air-2 testvis % nm libtestlib_shared.so --demangle
0000000000003f34 T _VisibilityDefault
0000000000003f20 t _VisibilityHidden
0000000000003eec T _VisibilityNoAttribute
0000000000003f00 t VisibilityStatic()
0000000000003f68 t VisibilityStaticInline()
0000000000003f48 T SomeInlineClass::SomeInlineMethod()
U _printf
(.venv) deweydunnington@Deweys-Air-2 testvis % g++ -c -fvisibility=hidden testlib.cc
(.venv) deweydunnington@Deweys-Air-2 testvis % g++ -shared -o libtestlib_shared.so testlib.o
(.venv) deweydunnington@Deweys-Air-2 testvis % nm libtestlib_shared.so --demangle
0000000000003f40 T _VisibilityDefault
0000000000003f2c t _VisibilityHidden
0000000000003ef8 t _VisibilityNoAttribute
0000000000003f0c t VisibilityStatic()
0000000000003f74 t VisibilityStaticInline()
0000000000003f54 t SomeInlineClass::SomeInlineMethod()
U _printftestlib.h
#include <stdio.h>
extern "C" {
static inline void VisibilityStaticInline(void) {
printf("something");
}
void VisibilityNoAttribute(void);
__attribute__((visibility("hidden"))) void VisibilityHidden(void);
__attribute__((visibility("default"))) void VisibilityDefault(void);
}testlib.cc
#include <cstdio>
#include "testlib.h"
class SomeInlineClass {
public:
void SomeInlineMethod() {
VisibilityStaticInline();
}
};
static void VisibilityStatic(void) {
SomeInlineClass cls;
cls.SomeInlineMethod();
}
void VisibilityNoAttribute(void) {
VisibilityStatic();
}
__attribute__((visibility("hidden"))) void VisibilityHidden(void) {
VisibilityNoAttribute();
}
__attribute__((visibility("default"))) void VisibilityDefault(void) {
VisibilityNoAttribute();
}| @@ -209,25 +209,25 @@ static const char* ConvertError(ArrowErrorCode errno_code) { | |||
|
|
|||
| int64_t nanoarrow_BytesAllocated() { return kBytesAllocated; } | |||
|
|
|||
| DLL_EXPORT const char* nanoarrow_CDataIntegration_ExportSchemaFromJson( | |||
| NANOARROW_EXPORT const char* nanoarrow_CDataIntegration_ExportSchemaFromJson( | |||
There was a problem hiding this comment.
nit: I think NANOARROW_DLL might be more appropriate (except for our own usage internally, this will most of the time expand to an import rather than an export).
(nanoarrow_BytesAllocated() also needs this attribute)
meson.build
Outdated
| cpp_args: device_defines, | ||
| gnu_symbol_visibility: 'hidden', |
There was a problem hiding this comment.
You can check the effect of this with nm --extern-only, but I am not sure that in nanoarrow's case (where we've been very careful to mark everything internal as static or put it in a header and mark it as static inline) this applies.
| #else | ||
| #define NANOARROW_DLL_IMPORT | ||
| #define NANOARROW_DLL_EXPORT | ||
| #define NANOARROW_DLL_LOCAL |
There was a problem hiding this comment.
I think the version that was in a previous commit was slightly better...the constraints here are:
- We want to make it easy for CMake or Meson to ensure the attributes are set correctly. Usually this would be by setting a target compile define such that
-DNANOARROW_BUILD_DLLor-DNANOARROW_CONSUME_DLLgets passed as a compile flag. - The effect of
NANOARROW_BUILD_DLLon _WIN32 vs otherwise is different (you have that here) - We want somebody to able to pass
-DNANOARROW_DLL=...to work around any mistake we made - There is no visibility attribute in gcc < 4 (you have that here)
There was a problem hiding this comment.
Sorry about that - misinterpreted your earlier comment as wanting to go back to just the first few commits.
FWIW I looked for a prior art in some other entries in the Meson WrapDB. Here's one for sqlitecpp:
docopt:
luajit:
I think the sqlitecpp approach is maybe the most relevant for us, i.e. we can try something like:
if host_machine.system() == 'windows' and get_option('default_library') == 'shared'
nanoarrow_args = ['-DNANOARROW_BUILD_DLL', '-DNANOARROW_EXPORT_DLL']
nanoarrow_dep_args = ['-DNANOARROW_BUILD_DLL']
endifInternally we use nanoarrow_args but third parties that use the dependency would automatically use the nanoarrow_dep_args. I think this approach is close to what you suggested, but instead of requiring consumers to provide -DNANOARROW_CONSUME_DLL we would internally use -DNANOARROW_EXPORT_DLL
| @@ -20,10 +20,20 @@ | |||
|
|
|||
| #include <stdint.h> | |||
|
|
|||
| #if defined(_MSC_VER) | |||
| #define DLL_EXPORT __declspec(dllexport) | |||
| #if defined _WIN32 || defined __CYGWIN__ | |||
There was a problem hiding this comment.
I think the C library still needs the attribute on Windows (but I can go through and do this in a later PR)
| @@ -76,19 +86,19 @@ struct ArrowArray { | |||
| #endif // ARROW_C_DATA_INTERFACE | |||
| #endif // ARROW_FLAG_DICTIONARY_ORDERED | |||
|
|
|||
| DLL_EXPORT const char* nanoarrow_CDataIntegration_ExportSchemaFromJson( | |||
| NANOARROW_DLL_EXPORT const char* nanoarrow_CDataIntegration_ExportSchemaFromJson( | |||
There was a problem hiding this comment.
Correcting myself here...they're header-only but some of them are public. (None of them end up as exported symbols, though)
| #define NANOARROW_DLL __declspec(dllexport) | ||
| #else | ||
| #define NANOARROW_DLL __declspec(dllimport) | ||
| #endif // (defined _WIN32 || defined __CYGWIN__) && defined(NANOARROW_BUILD_DLL) | ||
| #else |
There was a problem hiding this comment.
Maybe here: #elif !defined(NANOARROW_DLL) (i.e., let the user override this logic if they need to do something else)
| #define DLL_EXPORT | ||
| #define NANOARROW_DLL __declspec(dllimport) | ||
| #endif // defined(NANOARROW_EXPORT_DLL) | ||
| #elif !defined(NANOARROW_BUILD_DLL) |
There was a problem hiding this comment.
| #elif !defined(NANOARROW_BUILD_DLL) | |
| #elif !defined(NANOARROW_DLL) |
There was a problem hiding this comment.
Ah sorry - nice catch
I believe this will fix the issues we found with the latest release when trying to distribute via Windows