GBuella commented 7 years ago

The static library had type UTILITY, which was not usable in the link_target_libraries command. This patch creates a usable STATIC library, i.e. cmake understads it is a static library. Also, partial linking is done before hiding symbols.

Also, adding a test using the static library.

This change is

krzycz commented 7 years ago

Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

GBuella commented 7 years ago

I just tried what would happen without partial linking (linking all the object files into a single object file). This is what happens when trying to link the executable file_basic_using_static:

[ 83%] Linking CXX executable file_basic_using_static
libpmemfile_test.a(pmemfile_test.cpp.o): In function `test_pmemfile_stats_match(pmemfilepool*, unsigned int, unsigned int, unsigned int, unsigned int)':
/home/tej/code/pmemfile/tests/posix/pmemfile_test.cpp:51: undefined reference to `pmemfile_stats'
libpmemfile_test.a(pmemfile_test.cpp.o): In function `test_pmemfile_file_size(pmemfilepool*, pmemfile_file*)':
/home/tej/code/pmemfile/tests/posix/pmemfile_test.cpp:81: undefined reference to `pmemfile_fstat'
libpmemfile_test.a(pmemfile_test.cpp.o): In function `test_pmemfile_path_size(pmemfilepool*, char const*)':
/home/tej/code/pmemfile/tests/posix/pmemfile_test.cpp:92: undefined reference to `pmemfile_stat'
libpmemfile_test.a(pmemfile_test.cpp.o): In function `test_list_files[abi:cxx11](pmemfilepool*, pmemfile_file*, char const*, unsigned int)':
/home/tej/code/pmemfile/tests/posix/pmemfile_test.cpp:159: undefined reference to `pmemfile_readlinkat'
/home/tej/code/pmemfile/tests/posix/pmemfile_test.cpp:143: undefined reference to `pmemfile_fstatat'
libpmemfile_test.a(pmemfile_test.cpp.o): In function `test_list_files[abi:cxx11](pmemfilepool*, char const*)':
/home/tej/code/pmemfile/tests/posix/pmemfile_test.cpp:206: undefined reference to `pmemfile_getdents64'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
tests/posix/CMakeFiles/file_basic_using_static.dir/build.make:98: recipe for target 'tests/posix/file_basic_using_static' failed
make[2]: *** [tests/posix/file_basic_using_static] Error 1
CMakeFiles/Makefile2:1420: recipe for target 'tests/posix/CMakeFiles/file_basic_using_static.dir/all' failed
make[1]: *** [tests/posix/CMakeFiles/file_basic_using_static.dir/all] Error 2
Makefile:138: recipe for target 'all' failed
make: *** [all] Error 2

Actually, this problem seems to disappears when taking care of link order, i.e. changing src/tests/posix/CMakeLists.txt in the following way:

-       target_link_libraries(${name} ${posix_lib} pmemfile_test test_backtrace)
+       target_link_libraries(${name} pmemfile_test ${posix_lib} test_backtrace)

But for one thing, I believe, the library constructor stays in the finally linked executable only because there is a reference to some other symbol in the same object file.

codecov-io commented 7 years ago

Codecov Report

Merging #313 into master will decrease coverage by 1.2%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
- Coverage   78.22%   77.01%   -1.21%     
==========================================
  Files          90       90              
  Lines       13823    13823              
  Branches     1996     1996              
==========================================
- Hits        10813    10646     -167     
- Misses       2291     2426     +135     
- Partials      719      751      +32

Impacted Files	Coverage Δ
src/libpmemfile-posix/pmemfile-posix.c	`46.42% <0%> (-21.43%)`	:arrow_down:
tests/posix/rw/rw.cpp	`80.07% <0%> (-12.8%)`	:arrow_down:
src/libpmemfile-posix/lseek.c	`84.35% <0%> (-5.45%)`	:arrow_down:
src/libpmemfile-posix/block_array.c	`91.66% <0%> (-3.34%)`	:arrow_down:
src/libpmemfile-posix/data.c	`88.84% <0%> (-3.08%)`	:arrow_down:
src/libpmemfile-posix/file.c	`67.3% <0%> (-0.28%)`	:arrow_down:
src/libpmemfile-posix/rename.c	`91.2% <0%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a7eb5f0...f8f9803. Read the comment docs.

sarahjelinek commented 7 years ago

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

src/libpmemfile-posix/CMakeLists.txt, line 235 at r2 (raw file):

add_custom_command(OUTPUT pmemfile-posix_unscoped.o
      COMMAND ${CMAKE_LINKER} -r --whole-archive ${posix_unscoped_a}
                  -o pmemfile-posix_unscoped.o

CMAKE_LINKER is something that has to be specified on the command line? At least how I read its use online. Is this added to the README? What should be set here?

When I build the repo I get a static library but all the symbols show up. I am not sure what I am missing about how to build this differently.

tests/posix/CMakeLists.txt, line 116 at r2 (raw file):

  build_test(${name} pmemfile-posix_static ${srcs})
endfunction()

Add comments about what these tests are supposed to be doing.

Comments from Reviewable

GBuella commented 7 years ago

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

src/libpmemfile-posix/CMakeLists.txt, line 235 at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

CMAKE_LINKER is something that has to be specified on the command line? At least how I read its use online. Is this added to the README? What should be set here? When I build the repo I get a static library but all the symbols show up. I am not sure what I am missing about how to build this differently.

There is no need to manually specify it, unless one wishes to use something other than the default which cmake finds. The same scenario as with the compiler, and some other tools. POSIX requires a command named ld to be available, so cmake automatically finds at least that. But it better practice to allow the user to override it, just as we don't have clang or gcc hardwired here as compiler. Or, one can supply a toolchain for cross-compiling to cmake, and cmake will use the linker in that toolchain (e.g. someone building it on Windows, not that a lot of people are going to do that).

Do the symbols really show up after you build this repo? I mean, are they global? Then something is really wrong with this code here.

The next step, with objcopy, is meant to make the symbols local, as if they would have been originally declared with internal linkage - so they should not be available for anyone for linking. Except for the few public symbols, four of them, if I recall correctly.

I'v seen this pattern of creating a cmake static library, then linking its objects using the --whole-archive flag in numerous cmake project online. I'v seen pretty much the same thing in NVML too (and some other projects), and it seems to work there: https://github.com/pmem/nvml/blob/master/src/Makefile.inc#L213

$(LIB_AR_UNSCOPED): $(OBJS) $(EXTRA_OBJS) $(MAKEFILE_DEPS)
    $(LD) -o $@ -r $(OBJS) $(EXTRA_OBJS)

ifeq ($(LIB_MAP),)
$(LIB_AR_ALL): $(LIB_AR_UNSCOPED) $(MAKEFILE_DEPS)
    $(OBJCOPY) $< $@
else
$(LIB_AR_ALL): $(LIB_AR_UNSCOPED) $(LIB_MAP) $(MAKEFILE_DEPS)
    $(OBJCOPY) --localize-hidden `sed -n 's/^   *\([a-zA-Z0-9_]*\);$$/-G \1/p' $(LIB_MAP)` $< $@
endif

As I see it, basically the CMAKE_C_COMPILER cmake variable is supposed to play the role of the CC make variable, and CMAKE_LINKER is like the GNUmake LD variable. Additionally, cmake also looks for objcopy by default, and some other commonly used dev tools.

Comments from Reviewable

marcinslusarz commented 7 years ago

Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/libpmemfile-posix/CMakeLists.txt, line 235 at r2 (raw file):

Previously, GBuella (Gabor Buella) wrote…

There is no need to manually specify it, unless one wishes to use something other than the default which cmake finds. The same scenario as with the compiler, and some other tools. POSIX requires a command named `ld` to be available, so cmake automatically finds at least that. But it better practice to allow the user to override it, just as we don't have clang or gcc hardwired here as compiler. Or, one can supply a toolchain for cross-compiling to cmake, and cmake will use the linker in that toolchain (e.g. someone building it on Windows, not that a lot of people are going to do that). Do the symbols really show up after you build this repo? I mean, are they global? Then something is really wrong with this code here. The next step, with objcopy, is meant to make the symbols local, as if they would have been originally declared with internal linkage - so they should not be available for anyone for linking. Except for the few public symbols, four of them, if I recall correctly. I'v seen this pattern of creating a cmake static library, then linking its objects using the `--whole-archive` flag in numerous cmake project online. I'v seen pretty much the same thing in NVML too (and some other projects), and it seems to work there: https://github.com/pmem/nvml/blob/master/src/Makefile.inc#L213 ``` $(LIB_AR_UNSCOPED): $(OBJS) $(EXTRA_OBJS) $(MAKEFILE_DEPS) $(LD) -o $@ -r $(OBJS) $(EXTRA_OBJS) ifeq ($(LIB_MAP),) $(LIB_AR_ALL): $(LIB_AR_UNSCOPED) $(MAKEFILE_DEPS) $(OBJCOPY) $< $@ else $(LIB_AR_ALL): $(LIB_AR_UNSCOPED) $(LIB_MAP) $(MAKEFILE_DEPS) $(OBJCOPY) --localize-hidden `sed -n 's/^ *$[a-zA-Z0-9_]*$;$$/-G \1/p' $(LIB_MAP)` $< $@ endif ``` As I see it, basically the *CMAKE_C_COMPILER* cmake variable is supposed to play the role of the *CC* make variable, and *CMAKE_LINKER* is like the GNUmake *LD* variable. Additionally, cmake also looks for objcopy by default, and some other commonly used dev tools.

FTR, on Debian 9 both before and after this PR I see only expected symbols marked as external in nm output.

Comments from Reviewable

GBuella commented 7 years ago

Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/libpmemfile-posix/CMakeLists.txt, line 235 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

FTR, on Debian 9 both before and after this PR I see only expected symbols marked as external in nm output.

Yes, objcopy work before as well. There wasn't a lot of problem, until one actually tried to use that static library.

Comments from Reviewable

GBuella commented 7 years ago

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

tests/posix/CMakeLists.txt, line 116 at r2 (raw file):

Previously, sarahjelinek (Sarah Jelinek) wrote…

Add comments about what these tests are supposed to be doing.

Done.

Comments from Reviewable

GBuella commented 7 years ago

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

src/libpmemfile-posix/CMakeLists.txt, line 235 at r2 (raw file):

Previously, GBuella (Gabor Buella) wrote…

Yes, objcopy work before as well. There wasn't a lot of problem, until one actually tried to use that static library.

I meant "objcopy worked before this change too".

Comments from Reviewable

marcinslusarz commented 7 years ago

src/libpmemfile-posix/CMakeLists.txt, line 235 at r2 (raw file):

Previously, GBuella (Gabor Buella) wrote…

I meant "objcopy worked before this change too".

Yes, I verified static linking doesn't work on master and works with this PR.

Comments from Reviewable

marcinslusarz commented 7 years ago

Reviewed 1 of 2 files at r3, 1 of 1 files at r4. Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

sarahjelinek commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

src/libpmemfile-posix/CMakeLists.txt, line 235 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Yes, I verified static linking doesn't work on master and works with this PR.

I rebuilt this and I must have had old bits in my 'build' directory. It builds ok now and the global symbols are resolved.

Comments from Reviewable

sarahjelinek commented 7 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

pmem / pmemfile

Pmemfile posix static library #313

Codecov Report