This is an archive of the discontinued LLVM Phabricator instance.

Fix missing dependency in LibcUnitTest
ClosedPublic

Authored by gchatelet on Jan 24 2020, 2:41 AM.

Details

Summary

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

Diff Detail

Event Timeline

gchatelet created this revision.Jan 24 2020, 2:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2020, 2:41 AM
sivachandra accepted this revision.Jan 24 2020, 2:05 PM

Thanks for the fix.

This revision is now accepted and ready to land.Jan 24 2020, 2:05 PM
sivachandra requested changes to this revision.Jan 24 2020, 2:15 PM

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.

This revision now requires changes to proceed.Jan 24 2020, 2:15 PM

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.

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

abrachet added a subscriber: abrachet.EditedJan 25 2020, 3:48 PM

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 ...

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.

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 updated this revision to Diff 240427.Jan 26 2020, 3:13 AM
  • Address comments

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 accepted this revision.Jan 26 2020, 11:12 AM

LGTM, I would wait for Siva though.

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

LGTM, I would wait for Siva though.

Will do.

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 accepted this revision.Jan 26 2020, 1:47 PM
  • 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.

This revision is now accepted and ready to land.Jan 26 2020, 1:47 PM

LGTM, I would wait for Siva though.

Will do.

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.]

This revision was automatically updated to reflect the committed changes.