-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
build: enable Seastar to build shared and static libs in a single build #2427
Conversation
cea6dbc
to
0508fb3
Compare
@denesb could you please help review this change? it implements the proposal at scylladb/scylladb#2717 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I don't understand the Cmake changes in detail, but the concept as explained in the cover letter as well as the code in high level looks good.
thank your Botond, for your generous review. |
@scylladb/seastar-maint hello maintainers, could you help review this change? |
0508fb3
to
da15e0c
Compare
v2:
|
@scylladb/seastar-maint hello maintainers, could you help review this change? |
1 similar comment
@scylladb/seastar-maint hello maintainers, could you help review this change? |
If I understand correctly, we build the object files just once. Doesn't this degrade the performance of thread-local variables? It moves tls-model from initial-exec (fast) to global-dynamic. Or maybe global-dynamic. Need to examine the generated code to be sure. |
will do and report my findings here later on. |
da15e0c
to
b637404
Compare
@avikivity i checked the emitted elf files of |
v2:
|
Ok, this was due to my confusion. In d8a70b3 we switched shared libraries to initial-exec. I remembered we downgraded it from global-dynamic, but thought it wasn't to initial-exec. |
But looking at that commit, I see we have another different: -fvisibility=hidden. What happens now? Do shared libraries gain -fvisibility=hidden? Or do static libraries lose it? |
the commit of f34335e includes
so,
ahh. i see your point now. the latter, the static libraries lose it after this pull request, as long as we build the shared libraries and the static libraries in a single pass. do you think it's important? IMHO, it's not important at this moment. because:
but i agree that a smaller list of elf global symbols is desirable. for instance, it helps to reduce the size of DSO, shorten the load time of the DSO, etc. but because the commit message puts:
we cannot differentiate the public symbols from the internal ones, without adding the directives like IMHO, the recommended approach, although potentially tedious, is to:
but this is not in the scope of this change. and probably has lower priority as it's more about the DSO and is not trivial. what do you think? |
BTW, i created #2457 to track the enhancement for adding |
This comment was marked as resolved.
This comment was marked as resolved.
before this change, we respect the CMake variable named `BUILD_SHARED_LIBS`, and build shared libraries if it is set, build static libraries otherwise. but this model cannot fulfill the needs of a parent project which needs to build both static and shared seastar libraries with different configuration in a multi-config generator settings. as `BUILD_SHARED_LIBS` is a CMake variable which cannot be changed at build time, and hence cannot be assigned with different values using generator expression. so, in this change, we add two options - Seastar_BUILD_STATIC_LIBS - Seastar_BUILD_SHARED_LIBS to configure if the build should generated static libraries and shared libraries respectively. but `BUILD_SHARED_LIBS` is still supported, and the behavior is backward compatible. the only user-visible differences are, the libraries of - seastar - seastar_testing - seastar_perf_testing are now aliases of the targets which builds the static or shared libraries. so one cannot build "seastar" as a target anymore, but should specify which library to build: ``` $ cmake --build build --target seastar # does not work anymore $ cmake --build build --target seastar-static $ cmake --build build --target seastar-shared ``` the reason is the limit of an ALIAS library, which cannot be used as a target. but we still need to use a non-interface library to generate .pc file, where, for instance, we use `$<TARGET_FILE_NAME:seastar_testing>` for the library list of a certain Seastar library. but a CMake-based project including seastar can still link against it using "seastar" as a library. please note, in d8a70b3, we pass `--ftls-mode=initial-exec` when building the tree, to address the problem of recursive call of malloc when using seastar allocator. but we cannot use different compiling options when building both shared libraries and static libraries, so we always pass this option as long as `BUILD_SHARED_LIBS` OR `Seastar_BUILD_SHARED_LIBS` is set *and* when we are building dev/release modes. as these two modes enables the seastar allocator. this should not hurt the overall performance, as we are not using global-dynamic mode, but initial-exec. this is verified using ```console $ cmake \ -DCMAKE_CONFIGURATION_TYPES="Debug;RelWithDebInfo" \ -DCMAKE_CROSS_CONFIGS="Debug;RelWithDebInfo" \ -DCMAKE_DEFAULT_CONFIGS="Debug;RelWithDebInfo" \ -DSeastar_BUILD_SHARED_LIBS=ON \ -DSeastar_BUILD_STATIC_LIBS=ON \ -G "Ninja Multi-Config" -B build -S . $ cmake --build build --config RelWithDebInfo --target seastar-static $ readelf -r build/CMakeFiles/seastar-objects.dir/RelWithDebInfo/src/core/reactor.cc.o| \ grep -i tls | tr -s ' ' | cut -d' ' -f 3,5 | uniq R_X86_64_TPOFF32 __tls_guard R_X86_64_PLT32 _ZStlsRSoRKNSt15_[...] ``` and ```console $ cmake -DCMAKE_BUILD_TYPE="RelWithDebInfo" -G "Ninja" -B build -S . $ cmake --build build --target seastar-static $ readelf -r build/CMakeFiles/seastar-objects.dir/src/core/reactor.cc.o| \ grep -i tls | tr -s ' ' | cut -d' ' -f 3,5 | uniq R_X86_64_TPOFF32 __tls_guard R_X86_64_PLT32 _ZStlsRSoRKNSt15_[...] ``` so the TLS variables are using "TPOFF32" relocation type, in general, it implies the initial-exec TLS model. the PLT32 is not specific TLS, it's more related to external symbols linkages. Refs scylladb/scylladb#2717 Signed-off-by: Kefu Chai <[email protected]>
b637404
to
a8d7536
Compare
v3:
|
I'm not worried about shared object size. I don't want to lose optimization opportunities. Maybe we can replace it with -fvisibility-inlines-hidden -fno-semantic-interposition? (is -fno-sematic-interposition the default?) |
i see.
we cannot have
GCCsemantic-interposition is enabled by default. guess GCC intends to do the right thing as the ELF spec suggests. and it is off only when Clangsemantic-interposition is off by default for better performance. see llvm/llvm-project@d1fd723 . to be more specific, i am quoting the commit message here
so we do want to optimize EDIT, if we don't care about semantic interposition, i think we can disable it for all builds. |
created #2465 |
@avikivity could you please review my responses? i believe i've addressed all of your questions. |
@avikivity hi Avi, could you help re-review this change ? |
we still need to pass |
Also, even for a dynamically linked Seastar application, I guess that at least some of the runtime overhead should be possible to offset, by launching the application with |
thank you for your comment Laszlo. i agree with you that |
Right; AIUI, reading the GOT all the time cannot be eliminated for dynamically linked applications. |
before this change, we respect the CMake variable named
BUILD_SHARED_LIBS
, and build shared libraries if it is set, build static libraries otherwise. but this model cannot fulfill the needs of a parent project which needs to build both static and shared seastar libraries with different configuration in a multi-config generator settings. asBUILD_SHARED_LIBS
is a CMake variable which cannot be changed at build time, and hence cannot be assigned withdifferent values using generator expression.
so, in this change, we add two options
to configure if the build should generated static libraries and shared libraries respectively. but
BUILD_SHARED_LIBS
is still supported, and the behavior is backward compatible. the only user-visible differences are, the libraries ofare now aliases of the targets which builds the static or shared libraries. so one cannot build "seastar" as a target anymore, but should specify which library to build:
the reason is the limit of an ALIAS library, which cannot be used as a target. but we still need to use a non-interface library to generate .pc file, where, for instance, we use
$<TARGET_FILE_NAME:seastar_testing>
for the library list of acertain Seastar library.
but a CMake-based project including seastar can still link against it using "seastar" as a library.
please note, in d8a70b3, we pass
--ftls-mode=initial-exec
when building the tree, to address the problem of recursive call of malloc when using seastar allocator. but we cannot use different compiling options when building both shared libraries and static libraries, so we always pass this option as long asBUILD_SHARED_LIBS
ORSeastar_BUILD_SHARED_LIBS
is set and when we are building dev/release modes. as these two modes enables the seastar allocator. this should not hurt the overall performance, as we are not using global-dynamic mode, but initial-exec. this is verified usingand
so the TLS variables are using "TPOFF32" relocation type, in general, it implies the initial-exec TLS model. the PLT32 is not specific to TLS, it's more related to external symbols linkages.
Refs scylladb/scylladb#2717
Signed-off-by: Kefu Chai [email protected]