Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
@ 2020-06-11  0:25 HustonMmmavr
  2020-06-14 21:34 ` Alexander Turenko
  2020-06-15 21:20 ` Vladislav Shpilevoy
  0 siblings, 2 replies; 9+ messages in thread
From: HustonMmmavr @ 2020-06-11  0:25 UTC (permalink / raw)
  To: tarantool-patches, yaroslav.dynnikov, avtikhon, alexander.turenko

Removed definition and initialization of EXPORT_LIST variable at file
src/CMakeLists.txt. After patch 03790ac551 this variable is unused
(no reference to this variable after its initialization can be found
in whole project) and it is only misleading.

Closes #5066
---
I've builded tarantool before applying this changes and after.
Then I've checked difference in tarantool binary file symbols with 
nm and diff commands and there was no difference.

Issue: https://github.com/tarantool/tarantool/issues/5066
Branch: https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-5066-delete-unusued-export-list

@ChangeLog
- Cleanup src/CMakeLists.txt (gh-5066)

 src/CMakeLists.txt | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 7099e9bef..d86e6bfb1 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -213,33 +213,7 @@ add_subdirectory(box)
 set(TARANTOOL_C_FLAGS ${CMAKE_C_FLAGS} PARENT_SCOPE)
 set(TARANTOOL_CXX_FLAGS ${CMAKE_CXX_FLAGS} PARENT_SCOPE)
 
-set(EXPORT_LIST)
 if(BUILD_STATIC)
-    # for each static library we should find a corresponding shared library to
-    # parse and reexport library api functions
-    foreach(libstatic
-            ${READLINE_LIBRARIES}
-            ${CURL_LIBRARIES}
-            ${OPENSSL_LIBRARIES}
-            ${ICU_LIBRARIES})
-        if (${libstatic} MATCHES "lib[^/]+.a")
-            string(REGEX MATCH "lib[^/]+.a" libname ${libstatic})
-            string(REGEX REPLACE "\\.a$" "" libname ${libname})
-            string(REGEX REPLACE "^lib" "" libname ${libname})
-            find_library(SYMBOLS_LIB NAMES ${libname})
-            # add found library to export list
-            list(APPEND EXPORT_LIST ${SYMBOLS_LIB})
-            # set variable to allow rescan (CMake depended)
-            set(SYMBOLS_LIB "SYMBOLS_LIB-NOTFOUND")
-        elseif (${libstatic} STREQUAL bundled-libcurl OR
-                ${libstatic} STREQUAL bundled-ares)
-            message("We don't need to export symbols from statically linked ${libstatic}, skipped")
-        else()
-            message(WARNING "${libstatic} should be a static")
-        endif()
-    endforeach(libstatic)
-    string(REPLACE ";" " " EXPORT_LIST "${EXPORT_LIST}")
-
     if (HAVE_OPENMP)
         # Link libgomp explicitly to make it static. Avoid linking
         # against DSO version of libgomp, which implied by -fopenmp
-- 
2.26.2

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-11  0:25 [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt HustonMmmavr
@ 2020-06-14 21:34 ` Alexander Turenko
  2020-06-15 17:27   ` Mavr Huston
  2020-06-15 21:20 ` Vladislav Shpilevoy
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Turenko @ 2020-06-14 21:34 UTC (permalink / raw)
  To: HustonMmmavr; +Cc: tarantool-patches, Vladislav Shpilevoy, yaroslav.dynnikov

LGTM.

Vlad, can you look at the patch?

WBR, Alexander Turenko.

On Thu, Jun 11, 2020 at 03:25:10AM +0300, HustonMmmavr wrote:
> Removed definition and initialization of EXPORT_LIST variable at file
> src/CMakeLists.txt. After patch 03790ac551 this variable is unused
> (no reference to this variable after its initialization can be found
> in whole project) and it is only misleading.
> 
> Closes #5066

I would also mark it as follow up for #2971.

> ---
> I've builded tarantool before applying this changes and after.
> Then I've checked difference in tarantool binary file symbols with 
> nm and diff commands and there was no difference.
> 
> Issue: https://github.com/tarantool/tarantool/issues/5066
> Branch: https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-5066-delete-unusued-export-list
> 
> @ChangeLog
> - Cleanup src/CMakeLists.txt (gh-5066)

This change is not visible for a user, so I would not add a changelog
entry for it.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-14 21:34 ` Alexander Turenko
@ 2020-06-15 17:27   ` Mavr Huston
  0 siblings, 0 replies; 9+ messages in thread
From: Mavr Huston @ 2020-06-15 17:27 UTC (permalink / raw)
  To: Alexander Turenko
  Cc: tarantool-patches, Vladislav Shpilevoy, yaroslav.dynnikov

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

Thanks!
I've marked it as follow up for #2971 at commit message.

пн, 15 июн. 2020 г. в 00:35, Alexander Turenko <
alexander.turenko@tarantool.org>:

> LGTM.
>
> Vlad, can you look at the patch?
>
> WBR, Alexander Turenko.
>
> On Thu, Jun 11, 2020 at 03:25:10AM +0300, HustonMmmavr wrote:
> > Removed definition and initialization of EXPORT_LIST variable at file
> > src/CMakeLists.txt. After patch 03790ac551 this variable is unused
> > (no reference to this variable after its initialization can be found
> > in whole project) and it is only misleading.
> >
> > Closes #5066
>
> I would also mark it as follow up for #2971.
>
> > ---
> > I've builded tarantool before applying this changes and after.
> > Then I've checked difference in tarantool binary file symbols with
> > nm and diff commands and there was no difference.
> >
> > Issue: https://github.com/tarantool/tarantool/issues/5066
> > Branch:
> https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-5066-delete-unusued-export-list
> >
> > @ChangeLog
> > - Cleanup src/CMakeLists.txt (gh-5066)
>
> This change is not visible for a user, so I would not add a changelog
> entry for it.
>

[-- Attachment #2: Type: text/html, Size: 1848 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-11  0:25 [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt HustonMmmavr
  2020-06-14 21:34 ` Alexander Turenko
@ 2020-06-15 21:20 ` Vladislav Shpilevoy
  2020-06-17 15:29   ` Mavr Huston
  1 sibling, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-15 21:20 UTC (permalink / raw)
  To: HustonMmmavr, tarantool-patches, yaroslav.dynnikov, avtikhon,
	alexander.turenko

Hi!

On 11/06/2020 02:25, HustonMmmavr wrote:
> Removed definition and initialization of EXPORT_LIST variable at file
> src/CMakeLists.txt. After patch 03790ac551 this variable is unused
> (no reference to this variable after its initialization can be found
> in whole project) and it is only misleading.

Actually I am not sure it is not needed. Seems like purpose of this
exporter was the same as for static libraries in a non-static build -
not to allow to remove any public symbols of these libraries.

Because linker can eliminate some parts of static libraries, if sees
they are not used in the final executable.

So probably EXPORT_LIST for static build should be exported just like
exports.h, and it was missed in #2971.

> Closes #5066
> ---
> I've builded tarantool before applying this changes and after.
> Then I've checked difference in tarantool binary file symbols with 
> nm and diff commands and there was no difference.

Have you tried static build or normal build? Did you see the content
of EXPORT_LIST, what is there? Isn't this related to
https://github.com/tarantool/tarantool/issues/4559?

> Issue: https://github.com/tarantool/tarantool/issues/5066
> Branch: https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-5066-delete-unusued-export-list

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-15 21:20 ` Vladislav Shpilevoy
@ 2020-06-17 15:29   ` Mavr Huston
  2020-06-17 23:09     ` Vladislav Shpilevoy
  0 siblings, 1 reply; 9+ messages in thread
From: Mavr Huston @ 2020-06-17 15:29 UTC (permalink / raw)
  To: Vladislav Shpilevoy
  Cc: Alexander Turenko, tarantool-patches, yaroslav.dynnikov

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

EXPORT_LIST contains following libraries in case of static build (with
normal build it's empty):

/usr/lib/x86_64-linux-gnu/libreadline.so

/usr/lib/x86_64-linux-gnu/libcurses.so

/usr/lib/x86_64-linux-gnu/libform.so

/usr/lib/x86_64-linux-gnu/libtinfo.so

/usr/lib/x86_64-linux-gnu/libz.so

/opt/local/lib/libssl.so

/opt/local/lib/libcrypto.so

/usr/lib/x86_64-linux-gnu/libz.so

/opt/local/lib/libicui18n.so

/opt/local/lib/libicuuc.so

/opt/local/lib/libicudata.so


It doesn’t contains libcurl because it’s bundled statically. So it isn’t
related to https://github.com/tarantool/tarantool/issues/4559 but this
problem may be solved with next patch:
https://github.com/tarantool/tarantool/tree/rosik/refactor-static-build. At
this patch added flag --disble-symbos-hiding (
https://github.com/tarantool/tarantool/blob/rosik/refactor-static-build/cmake/BuildLibCURL.cmake#L93
) at building libcurl and after that most of libcurl symbols are visible
from tarantool binary

вт, 16 июн. 2020 г. в 00:20, Vladislav Shpilevoy <v.shpilevoy@tarantool.org
>:

> Hi!
>
> On 11/06/2020 02:25, HustonMmmavr wrote:
> > Removed definition and initialization of EXPORT_LIST variable at file
> > src/CMakeLists.txt. After patch 03790ac551 this variable is unused
> > (no reference to this variable after its initialization can be found
> > in whole project) and it is only misleading.
>
> Actually I am not sure it is not needed. Seems like purpose of this
> exporter was the same as for static libraries in a non-static build -
> not to allow to remove any public symbols of these libraries.
>
> Because linker can eliminate some parts of static libraries, if sees
> they are not used in the final executable.
>
> So probably EXPORT_LIST for static build should be exported just like
> exports.h, and it was missed in #2971.
>
> > Closes #5066
> > ---
> > I've builded tarantool before applying this changes and after.
> > Then I've checked difference in tarantool binary file symbols with
> > nm and diff commands and there was no difference.
>
> Have you tried static build or normal build? Did you see the content
> of EXPORT_LIST, what is there? Isn't this related to
> https://github.com/tarantool/tarantool/issues/4559?
>
> > Issue: https://github.com/tarantool/tarantool/issues/5066
> > Branch:
> https://github.com/tarantool/tarantool/tree/HustonMmmavr/gh-5066-delete-unusued-export-list
>

[-- Attachment #2: Type: text/html, Size: 6579 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-17 15:29   ` Mavr Huston
@ 2020-06-17 23:09     ` Vladislav Shpilevoy
  2020-06-19 13:02       ` Yaroslav Dynnikov
  0 siblings, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-17 23:09 UTC (permalink / raw)
  To: Mavr Huston; +Cc: Alexander Turenko, tarantool-patches, yaroslav.dynnikov

On 17/06/2020 17:29, Mavr Huston wrote:
> 
> EXPORT_LIST contains following libraries in case of static build (with normal build it's empty):
> 
> /usr/lib/x86_64-linux-gnu/libreadline.so
> 
> /usr/lib/x86_64-linux-gnu/libcurses.so
> 
> /usr/lib/x86_64-linux-gnu/libform.so
> 
> /usr/lib/x86_64-linux-gnu/libtinfo.so
> 
> /usr/lib/x86_64-linux-gnu/libz.so
> 
> /opt/local/lib/libssl.so
> 
> /opt/local/lib/libcrypto.so
> 
> /usr/lib/x86_64-linux-gnu/libz.so
> 
> /opt/local/lib/libicui18n.so
> 
> /opt/local/lib/libicuuc.so
> 
> /opt/local/lib/libicudata.so
> 
> 
> It doesn’t contains libcurl because it’s bundled statically. So it isn’t related to https://github.com/tarantool/tarantool/issues/4559but this problem may be solved with next patch: https://github.com/tarantool/tarantool/tree/rosik/refactor-static-build. At this patch added flag --disble-symbos-hiding (https://github.com/tarantool/tarantool/blob/rosik/refactor-static-build/cmake/BuildLibCURL.cmake#L93) at building libcurl and after that most of libcurl symbols are visible from tarantool binary

The problem is not in the hiding. It is about removal. Not used symbols from
static libs may be removed from the final executable. Hide or not hide rules
are applied to what is left. That is the single reason why we had exports file
before 2971 and have exports.h and exports.c now. You can try it by yourself -
just add an unused function to lib/bit to bit.h and bit.c. And don't use it
anywhere. You may even add 'export' to it, or change visibility rules using
__attribute__ - it does not matter. If the function is not used and is not
added to exports.h, you won't see it in the executable. (At least it was so
last time I tried, works not with all libs, but with lib/bit it worked).

Seems EXPORT_LIST was used to extract all symbols from the static libs and
force their exposure + forbid their removal. Here the symbols were retrieved:
https://github.com/tarantool/tarantool/commit/03790ac5510648d1d9648bb2281857a7992d0593#diff-6b9c867c54f1a1b792de45d5262f1dcfL20-L25

Here the libs were passed to mkexports:
https://github.com/tarantool/tarantool/commit/03790ac5510648d1d9648bb2281857a7992d0593#diff-95e351a3805a1dafa85bf20b81d086e6L253-L260

We probably should resurrect that part. Rename the current exports.h to exports.h.in
and generate exports.h during cmake. Like it was done for exports before 2971. To
forbid symbols removal. Not to unhide them.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-17 23:09     ` Vladislav Shpilevoy
@ 2020-06-19 13:02       ` Yaroslav Dynnikov
  2020-06-19 23:39         ` Vladislav Shpilevoy
  0 siblings, 1 reply; 9+ messages in thread
From: Yaroslav Dynnikov @ 2020-06-19 13:02 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko

[-- Attachment #1: Type: text/plain, Size: 5724 bytes --]

I've researched the linking process and here is what I've found.

First of all, let's talk about symbols removal. Here is an interesting note
on
how it works:
https://stackoverflow.com/questions/55130965/when-and-why-would-the-c-linker-exclude-unused-symbols
One of the basic important concepts is an ELF section. It's a chunk of data
(assembly code in our case) that linker operates on. A section may contain
code
of one or more functions (depending on build arguments), but it can't be
split
during linking.

Usually, when we build an object file with `gcc libx.c -c -o libx.o`, all
functions from libx.c go into the single ".text" section. This single object
file is usually archived with others into single libx.a, which is later
processed during main executable linking.

By default (but I can't provide the proof), when we call `gcc -lx` linker
operates on object files from the archive - if at least one symbol is used,
whole .o (not .a) is included in the resulting binary. There are also two
linker
options that influence this behavior. At first, there is
`-Wl,--whole-archive`,
which makes it to include whole `.a` instead of `.o` granularity. Secondly,
there is
`-Wl,--gc-sections` which could remove unused sections, but in basic example
it's useless since all symbols from .o belong to the same .text section. To
make
`--gc-sections` have an effect one should compile object files with
`-ffunction-sections` flag. It'll generate a separate section for every
function
so the linker could gc unused ones.

See:
```console$ cat libx.c
#include <stdio.h>

void fA() {
        printf("fA is here\n");
}
void fB() {
        printf("fB is here\n");
}
$ gcc libx.c -c -o libx.o -ffunction-sections
$ readelf -S libx.o | grep .text
  [ 1] .text             PROGBITS         0000000000000000  00000040
  [ 5] .text.fA          PROGBITS         0000000000000000  00000056
  [ 6] .rela.text.fA     RELA             0000000000000000  000002d8
  [ 7] .text.fB          PROGBITS         0000000000000000  0000006d
  [ 8] .rela.text.fB     RELA             0000000000000000  00000308
```

Now let's move to the `libbit` which Vlad mentioned.
I've investigated how compiler options influence the resulting binary.
Unused
functions from bit.c are really remover, but only with Release flags, and
here
is why:

There are only 2 functions implemented in bit.c, and both are unused. All
the
others are inlines in bit.h and externs from luajit. When tarantool is
built in
debug mode, the inlining is off, so other modules truly link to the bit.o
and
all symbols remain including unused functions. But if we specify -O2 flag,
inlining takes place, and all the symbols from bit.o becomes unused, so the
linker drops the whole object file.

Finally, speaking about this patch, my proposal is to merge this PR as is.
And since we know how to manage linking, other problems can be solved
separately
(if they ever occur).

Best regards
Yaroslav Dynnikov

On Thu, 18 Jun 2020 at 02:09, Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
wrote:

> On 17/06/2020 17:29, Mavr Huston wrote:
> >
> > EXPORT_LIST contains following libraries in case of static build (with
> normal build it's empty):
> >
> > /usr/lib/x86_64-linux-gnu/libreadline.so
> >
> > /usr/lib/x86_64-linux-gnu/libcurses.so
> >
> > /usr/lib/x86_64-linux-gnu/libform.so
> >
> > /usr/lib/x86_64-linux-gnu/libtinfo.so
> >
> > /usr/lib/x86_64-linux-gnu/libz.so
> >
> > /opt/local/lib/libssl.so
> >
> > /opt/local/lib/libcrypto.so
> >
> > /usr/lib/x86_64-linux-gnu/libz.so
> >
> > /opt/local/lib/libicui18n.so
> >
> > /opt/local/lib/libicuuc.so
> >
> > /opt/local/lib/libicudata.so
> >
> >
> > It doesn’t contains libcurl because it’s bundled statically. So it isn’t
> related to https://github.com/tarantool/tarantool/issues/4559but this
> problem may be solved with next patch:
> https://github.com/tarantool/tarantool/tree/rosik/refactor-static-build.
> At this patch added flag --disble-symbos-hiding (
> https://github.com/tarantool/tarantool/blob/rosik/refactor-static-build/cmake/BuildLibCURL.cmake#L93)
> at building libcurl and after that most of libcurl symbols are visible from
> tarantool binary
>
> The problem is not in the hiding. It is about removal. Not used symbols
> from
> static libs may be removed from the final executable. Hide or not hide
> rules
> are applied to what is left. That is the single reason why we had exports
> file
> before 2971 and have exports.h and exports.c now. You can try it by
> yourself -
> just add an unused function to lib/bit to bit.h and bit.c. And don't use it
> anywhere. You may even add 'export' to it, or change visibility rules using
> __attribute__ - it does not matter. If the function is not used and is not
> added to exports.h, you won't see it in the executable. (At least it was so
> last time I tried, works not with all libs, but with lib/bit it worked).
>
> Seems EXPORT_LIST was used to extract all symbols from the static libs and
> force their exposure + forbid their removal. Here the symbols were
> retrieved:
>
> https://github.com/tarantool/tarantool/commit/03790ac5510648d1d9648bb2281857a7992d0593#diff-6b9c867c54f1a1b792de45d5262f1dcfL20-L25
>
> Here the libs were passed to mkexports:
>
> https://github.com/tarantool/tarantool/commit/03790ac5510648d1d9648bb2281857a7992d0593#diff-95e351a3805a1dafa85bf20b81d086e6L253-L260
>
> We probably should resurrect that part. Rename the current exports.h to
> exports.h.in
> and generate exports.h during cmake. Like it was done for exports before
> 2971. To
> forbid symbols removal. Not to unhide them.
>

[-- Attachment #2: Type: text/html, Size: 7348 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-19 13:02       ` Yaroslav Dynnikov
@ 2020-06-19 23:39         ` Vladislav Shpilevoy
  2020-06-23 20:31           ` Yaroslav Dynnikov
  0 siblings, 1 reply; 9+ messages in thread
From: Vladislav Shpilevoy @ 2020-06-19 23:39 UTC (permalink / raw)
  To: Yaroslav Dynnikov; +Cc: tarantool-patches, Alexander Turenko

Hi! Thanks for the investigation!

On 19/06/2020 15:02, Yaroslav Dynnikov wrote:
> I've researched the linking process and here is what I've found.
> 
> First of all, let's talk about symbols removal. Here is an interesting note on
> how it works: https://stackoverflow.com/questions/55130965/when-and-why-would-the-c-linker-exclude-unused-symbols
> One of the basic important concepts is an ELF section. It's a chunk of data
> (assembly code in our case) that linker operates on. A section may contain code
> of one or more functions (depending on build arguments), but it can't be split
> during linking.
> 
> Usually, when we build an object file with `gcc libx.c -c -o libx.o`, all
> functions from libx.c go into the single ".text" section. This single object
> file is usually archived with others into single libx.a, which is later
> processed during main executable linking.
> 
> By default (but I can't provide the proof), when we call `gcc -lx` linker
> operates on object files from the archive - if at least one symbol is used,
> whole .o (not .a) is included in the resulting binary. There are also two linker
> options that influence this behavior. At first, there is `-Wl,--whole-archive`,
> which makes it to include whole `.a` instead of `.o` granularity. Secondly, there is
> `-Wl,--gc-sections` which could remove unused sections, but in basic example
> it's useless since all symbols from .o belong to the same .text section. To make
> `--gc-sections` have an effect one should compile object files with
> `-ffunction-sections` flag. It'll generate a separate section for every function
> so the linker could gc unused ones.
> 
> See:
> ```console$ cat libx.c
> #include <stdio.h>
> 
> void fA() {
>         printf("fA is here\n");
> }
> void fB() {
>         printf("fB is here\n");
> }
> $ gcc libx.c -c -o libx.o -ffunction-sections
> $ readelf -S libx.o | grep .text
>   [ 1] .text             PROGBITS         0000000000000000  00000040
>   [ 5] .text.fA          PROGBITS         0000000000000000  00000056
>   [ 6] .rela.text.fA     RELA             0000000000000000  000002d8
>   [ 7] .text.fB          PROGBITS         0000000000000000  0000006d
>   [ 8] .rela.text.fB     RELA             0000000000000000  00000308
> ```
> 
> Now let's move to the `libbit` which Vlad mentioned.
> I've investigated how compiler options influence the resulting binary. Unused
> functions from bit.c are really remover, but only with Release flags, and here
> is why:
> 
> There are only 2 functions implemented in bit.c, and both are unused. All the
> others are inlines in bit.h and externs from luajit. When tarantool is built in
> debug mode, the inlining is off, so other modules truly link to the bit.o and
> all symbols remain including unused functions. But if we specify -O2 flag,
> inlining takes place, and all the symbols from bit.o becomes unused, so the
> linker drops the whole object file.

So do you mean, that if a library consists of more than 1 C file (and builds
more than one .o file), and functions from some of them are not used, these
.o files and their code will be removed?

If it is true, it does not look like green light for the patch and requires
more experiments. For example, try to add a new library with several C files,
use only some of them in the Tarantool executable (just add to exports.h),
and see if code from unused .o files is removed even when they are built into
.a file.

However, as I said in private - I am ok with pushing this patch now. I just
don't see why is it necessary. Why is it so important to push it? Push just
for push? Just to close an issue? It does not improve anything, EXPORT_LIST
still may appear to be useful along with some other things I removed in 2971,
when I didn't think of the static build.

> Finally, speaking about this patch, my proposal is to merge this PR as is.
> And since we know how to manage linking, other problems can be solved separately
> (if they ever occur).
> 
> Best regards
> Yaroslav Dynnikov

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt
  2020-06-19 23:39         ` Vladislav Shpilevoy
@ 2020-06-23 20:31           ` Yaroslav Dynnikov
  0 siblings, 0 replies; 9+ messages in thread
From: Yaroslav Dynnikov @ 2020-06-23 20:31 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko

[-- Attachment #1: Type: text/plain, Size: 5293 bytes --]

Hi, Vlad.

Sorry for the late answer. I was going to recheck ideas you've proposed,
but didn't have time for that.
So my answer is based on my knowledge only.

On Sat, 20 Jun 2020, 02:39 Vladislav Shpilevoy, <v.shpilevoy@tarantool.org>
wrote:

> Hi! Thanks for the investigation!
>
> On 19/06/2020 15:02, Yaroslav Dynnikov wrote:
> > I've researched the linking process and here is what I've found.
> >
> > First of all, let's talk about symbols removal. Here is an interesting
> note on
> > how it works: https://stackoverflow.com/questions/55130965/when-drop
> unuseddrop unusedand-why-would-the-c-linker-exclude-unused-symbols
> <https://stackoverflow.com/questions/55130965/when-and-why-would-the-c-linker-exclude-unused-symbols>
> > One of the basic important concepts is an ELF section. It's a chunk of
> data
> > (assembly code in our case) that linker operates on. A section may
> contain code
> > of one or more functions (depending on build arguments), but it can't be
> split
> > during linking.
> >
> > Usually, when we build an object file with `gcc libx.c -c -o libx.o`, all
> > functions from libx.c go into the single ".text" section. This single
> object
> > file is usually archived with others into single libx.a, which is later
> > processed during main executable linking.
> >
> > By default (but I can't provide the proof), when we call `gcc -lx` linker
> > operates on object files from the archive - if at least one symbol is
> used,
> > whole .o (not .a) is included in the resulting binary. There are also
> two linker
> > options that influence this behavior. At first, there is
> `-Wl,--whole-archive`,
> > which makes it to include whole `.a` instead of `.o` granularity.
> Secondly, there is
> > `-Wl,--gc-sections` which could remove unused sections, but in basic
> example
> > it's useless since all symbols from .o belong to the same .text section.
> To make
> > `--gc-sections` have an effect one should compile object files with
> > `-ffunction-sections` flag. It'll generate a separate section for every
> function
> > so the linker could gc unused ones.
> >
> > See:
> > ```console$ cat libx.c
> > #include <stdio.h>
> >
> > void fA() {
> >         printf("fA is here\n");
> > }
> > void fB() {
> >         printf("fB is here\n");
> > }
> > $ gcc libx.c -c -o libx.o -ffunction-sections
> > $ readelf -S libx.o | grep .text
> >   [ 1] .text             PROGBITS         0000000000000000  00000040
> >   [ 5] .text.fA          PROGBITS         0000000000000000  00000056
> >   [ 6] .rela.text.fA     RELA             0000000000000000  000002d8
> >   [ 7] .text.fB          PROGBITS         0000000000000000  0000006d
> >   [ 8] .rela.text.fB     RELA             0000000000000000  00000308
> > ```
> >
> > Now let's move to the `libbit` which Vlad mentioned.
> > I've investigated how compiler options influence the resulting binary.
> Unused
> > functions from bit.c are really remover, but only with Release flags,
> and here
> > is why:
> >
> > There are only 2 functions implemented in bit.c, and both are unused.
> All the
> > others are inlines in bit.h and externs from luajit. When tarantool is
> built in
> > debug mode, the inlining is off, so other modules truly link to the
> bit.o and
> > all symbols remain including unused functions. But if we specify -O2
> flag,
> > inlining takes place, and all the symbols from bit.o becomes unused, so
> the
> > linker drops the whole object file.
>
> So do you mean, that if a library consists of more than 1 C file (and
> builds
> more than one .o file), and functions from some of them are not used, these
> .o files and their code will be removed?
>

Exactly. Moreover, it depends on the gcc command syntax.
Suppose we have libx.a with two object files: fA.o and fB.o. Function from
fA is used, and fB isn't.
Then `gcc main.c libx.a` will produce binary with both fA and fB.
While `gcc main.c -lx` will include fA only, and unused fB will be dropped.
It's also mentioned in the SO question
<https://stackoverflow.com/questions/55130965/when-and-why-would-the-c-linker-exclude-unused-symbols>
I linked in the previous message.


> If it is true, it does not look like green light for the patch and requires
> more experiments. For example, try to add a new library with several C
> files,
> use only some of them in the Tarantool executable (just add to exports.h),
> and see if code from unused .o files is removed even when they are built
> into
> .a file.
>

I guess the solution will be adding --whole-archive flag. But i'm not sure
yet.


>
> However, as I said in private - I am ok with pushing this patch now. I just
> don't see why is it necessary. Why is it so important to push it? Push just
> for push? Just to close an issue? It does not improve anything, EXPORT_LIST
> still may appear to be useful along with some other things I removed in
> 2971,
> when I didn't think of the static build.
>

You're right, it's not that important. I've already told Alex Barulev that
we'll postpone this
cleanup until we finish with static build refactoring.


>
> > Finally, speaking about this patch, my proposal is to merge this PR as
> is.
> > And since we know how to manage linking, other problems can be solved
> separately
> > (if they ever occur).
> >
> > Best regards
> > Yaroslav Dynnikov
>

[-- Attachment #2: Type: text/html, Size: 6889 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-06-23 20:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-11  0:25 [Tarantool-patches] [PATCH] cmake: cleanup src/CMakeLists.txt HustonMmmavr
2020-06-14 21:34 ` Alexander Turenko
2020-06-15 17:27   ` Mavr Huston
2020-06-15 21:20 ` Vladislav Shpilevoy
2020-06-17 15:29   ` Mavr Huston
2020-06-17 23:09     ` Vladislav Shpilevoy
2020-06-19 13:02       ` Yaroslav Dynnikov
2020-06-19 23:39         ` Vladislav Shpilevoy
2020-06-23 20:31           ` Yaroslav Dynnikov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox