Windows static pr #454
Windows static pr #454
Conversation
| @@ -540,6 +540,7 @@ if(SEAL_BUILD_SEAL_C) | |||
| seal_set_version(sealc) | |||
| if(SEAL_BUILD_STATIC_SEAL_C) | |||
| seal_set_version_filename(sealc) | |||
| add_compile_definitions(SEAL_BUILD_STATIC_SEAL_C) | |||
There was a problem hiding this comment.
@rickwebiii I'm not familiar with this command. What does it do?
There was a problem hiding this comment.
It tells the compiler to #define SEAL_BUILD_STATIC_SEAL_C. In gcc/clang, this passes -D SEAL_BUILD_STATIC_SEAL_C, in msvc it passes /D SEAL_BUILD_STATIC_SEAL_C.
There was a problem hiding this comment.
Does it work for a downstream app? It cannot see #define SEAL_BUILD_STATIC_SEAL_C. So when a downstream app includes seal/c/batchencoder.h that includes seal/c/defines.h, __declspec(dllimport) is always used, right?
There was a problem hiding this comment.
Oh, good point. Users would have to add this line to their CMAKE file or some equivalent, which I agree isn't great. I'll do some investigation and try to find how to make libraries both statically and dynamically linkable in Windows.
There was a problem hiding this comment.
I cannot approve this PR right now. It's really useful though. Shall we keep this PR open? We will release a new version of SEAL soon. We can always rebase this PR to a new version -- there shouldn't be hard conflicts.
There was a problem hiding this comment.
I need to test this, but it looks like you can always omit declspec(dllimport) and the headers would work for both static and dynamic linking.
The cost of doing so appears to be a single jmp instruction. I'd expect this cost to be a clock cycle, as unconditional branch prediction is trivial and I'd imagine most thunks are near each other in memory, so a bunch fit into an icache line. Since most interesting things you do in SEAL are O(us to ms), this tradeoff is probably fine. dllimport allows you to avoid a thunk in many cases. You only incur this cost with dynamic linking; calling a statically linked function doesn't require a thunk.
rickwebiii
force-pushed
the
windows_static_pr
branch
from
4911eba
to
828a487
Compare
This PR might be a bit controversial
This PR does the following:
#defineingCOR_E_IO, andCOR_E_INVALIDOPERATIONto whatever the values are on Linux. Philosophically, the C bindings shouldn't depend on .NET because you might not be building for C# (i.e. I'm building Rust bindings).SEAL_BUILD_STATIC_SEAL_C=ON