Fix missing dependency in LibcUnitTest

[gchatelet]

Summary

LibcUnitTest is missing a dependency on LLVMSupport. This prevents building with shared libraries.

[sivachandra]

Thanks for the fix.

[sivachandra]

Sorry, I take my acceptance back. I do not think this is required. If you rebase to HEAD of master, it should work without this change. I think, when you reverted the changes to LLVMLibCRules.cmake, you removed LLVMSupport from the list of link libraries for add_libc_unittest targets.

[gchatelet]

In D73337#1839664, @sivachandra wrote:

Sorry, I take my acceptance back. I do not think this is required. If you rebase to HEAD of master, it should work without this change. I think, when you reverted the changes to LLVMLibCRules.cmake, you removed LLVMSupport from the list of link libraries for add_libc_unittest targets.

No this is required, compiling a fresh origin/master leads to a bunch of errors of the style

ld.lld: error: undefined symbol: llvm::raw_ostream::operator<<(unsigned long long)
>>> referenced by Test.cpp:46 (/redacted/llvm-project/libc/utils/UnitTest/Test.cpp:46)
>>>               projects/libc/utils/UnitTest/CMakeFiles/LibcUnitTest.dir/Test.cpp.o:(bool llvm_libc::testing::internal::test<unsigned long long>(llvm_libc::testing::RunContext&, llvm_libc::testing::TestCondition, unsigned long long, unsigned long long, char const*, char const*, char const*, unsigned long))

This is because the library itself depends on Support to be able to be linked.

[sivachandra]

I am unable to reproduce even with -DBUILD_SHARED_LIBS=ON. Can you share your CMake command?

[abrachet]

This works without this patch for me too. I ran cmake ../llvm -DLLVM_ENABLE_PROJECTS=libc -G Ninja

$ cmake --version
cmake version 3.16.3
$ ninja --version
1.7.2

Update: (sorry for all the noise)

It does actually break with -DBUILD_SHARED_LIBS=On for me. It seems that if you just rerun cmake with -DBUILD_SHARED_LIBS=On it won't actually change unless you first remove the previously generated files.

/llvm-project/build> rm -rf *
/llvm-project/build> cmake ../llvm  -DLLVM_ENABLE_PROJECTS=libc -G Ninja -DBUILD_SHARED_LIBS=On
/llvm-project/build> ninja check-libc  # broken
[207/267] Linking CXX shared library lib/libLibcUnitTest.so.11git
FAILED: lib/libLibcUnitTest.so.11git
... linker errors ...

[abrachet]

I think this patch is still correct to add this, though. Even if the unittest itself will be linked to Support and also linked against LibcUnitTest and even if LibcUnitTest will never be used outside of that context, it would still make sense for LibcUnitTest to link against everything it needs. At worst this can't hurt.

[sivachandra]

In D73337#1840618, @abrachet wrote:

I think this patch is still correct to add this, though. Even if the unittest itself will be linked to Support and also linked against LibcUnitTest and even if LibcUnitTest will never be used outside of that context, it would still make sense for LibcUnitTest to link against everything it needs. At worst this can't hurt.

If you want to keep it here, then you should remove it from the list of the test exe's link libraries. I have verified that such a scheme works. So, with that change included, this change is alright with me.

[gchatelet]

• Address comments

[gchatelet]

In D73337#1840644, @sivachandra wrote:

In D73337#1840618, @abrachet wrote:

I think this patch is still correct to add this, though. Even if the unittest itself will be linked to Support and also linked against LibcUnitTest and even if LibcUnitTest will never be used outside of that context, it would still make sense for LibcUnitTest to link against everything it needs. At worst this can't hurt.

If you want to keep it here, then you should remove it from the list of the test exe's link libraries. I have verified that such a scheme works. So, with that change included, this change is alright with me.

So for completeness here was the reproducer

cd llvm-project
rm -rf /tmp/build
cmake -B/tmp/build -Sllvm -DLLVM_ENABLE_PROJECTS=libc -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Debug
make -C /tmp/build -j

The error happens if you build everything.

I've updated the patch.

[abrachet]

LGTM, I would wait for Siva though.

In D73337#1840856, @gchatelet wrote:

So for completeness here was the reproducer

cd llvm-project
rm -rf /tmp/build
cmake -B/tmp/build -Sllvm -DLLVM_ENABLE_PROJECTS=libc -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Debug
make -C /tmp/build -j

The error happens if you build everything.

I've updated the patch.

A little bit off topic, can you successfully build with make? A few weeks ago when I tried cmake was producing invalid Makefiles but ninja was fine. It broke for Siva too https://reviews.llvm.org/D72353#1808817

[gchatelet]

In D73337#1841007, @abrachet wrote:

LGTM, I would wait for Siva though.

Will do.

In D73337#1840856, @gchatelet wrote:

A little bit off topic, can you successfully build with make? A few weeks ago when I tried cmake was producing invalid Makefiles but ninja was fine. It broke for Siva too https://reviews.llvm.org/D72353#1808817

Indeed if I compile with make I have the following error, not the same as yours though.

make[2]: *** No rule to make target 'projects/libc/src/sys/mman/linux/munmap.o', needed by 'projects/libc/lib/libllvmlibc.a'.  Stop.
make[2]: Leaving directory '/tmp/build'
make[1]: *** [CMakeFiles/Makefile2:23563: projects/libc/lib/CMakeFiles/llvmlibc.dir/all] Error 2

I first thought it was an issue with -j option but even when building without the option it still fails with the same error message. It compiles fine with Ninja though.

[sivachandra]

In D73337#1840853, @gchatelet wrote:

• Address comments

Thanks for you patience. I really wish the LINK_COMPONENTS option was required for static and dynamic libraries. But, if dynamic libraries is a must going forward, I think I should include it in my normal patch testing.

[sivachandra]

In D73337#1841025, @gchatelet wrote:

In D73337#1841007, @abrachet wrote:

LGTM, I would wait for Siva though.

Will do.

In D73337#1840856, @gchatelet wrote:

A little bit off topic, can you successfully build with make? A few weeks ago when I tried cmake was producing invalid Makefiles but ninja was fine. It broke for Siva too https://reviews.llvm.org/D72353#1808817

Indeed if I compile with make I have the following error, not the same as yours though.

make[2]: *** No rule to make target 'projects/libc/src/sys/mman/linux/munmap.o', needed by 'projects/libc/lib/libllvmlibc.a'.  Stop.
make[2]: Leaving directory '/tmp/build'
make[1]: *** [CMakeFiles/Makefile2:23563: projects/libc/lib/CMakeFiles/llvmlibc.dir/all] Error 2

I first thought it was an issue with -j option but even when building without the option it still fails with the same error message. It compiles fine with Ninja though.

I have not yet got to digging into the problems with make. As I said earlier, it is very likely a CMake problem/limitation. [Just guessing that we are using some feature which is pushing the limits of CMake's dependency analysis.]

libc/cmake/modules/LLVMLibCRules.cmake

function(add_libc_unittest target_name)
   set_target_properties(${target_name} PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
 
   add_dependencies(
     ${target_name}
     ${LIBC_UNITTEST_DEPENDS}
     gtest
   )
 
-  target_link_libraries(${target_name} PRIVATE LibcUnitTest LLVMSupport)
+  target_link_libraries(${target_name} PRIVATE LibcUnitTest)
 
   add_custom_command(
     TARGET ${target_name}
     POST_BUILD
     COMMAND $<TARGET_FILE:${target_name}>
   )
   if(LIBC_UNITTEST_SUITE)
     add_dependencies(

libc/utils/UnitTest/CMakeLists.txt

 add_llvm_library(
   LibcUnitTest
   Test.cpp
   Test.h
+  LINK_COMPONENTS Support
 )