This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Final dependency cleanup patch!
ClosedPublic

Authored by beanz on Jan 31 2017, 3:18 PM.

Details

Summary

This patch removes the over-specified dependencies from LLDBDependencies and instead relies on the dependencies as expressed in each library and tool.

This also removes the library looping in favor of allowing CMake to do its thing. I've tested this patch on Darwin, and found no issues, but since linker semantics vary by system I'll also work on testing it on other platforms too.

Help testing would be greatly appreciated.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz created this revision.Jan 31 2017, 3:18 PM
labath edited edge metadata.Jan 31 2017, 3:23 PM

I'll give this a spin on linux now.

When linking liblldb.so:
/usr/bin/ld.gold: error: cannot find -llldbPluginDynamicLoaderDarwinKernel

beanz updated this revision to Diff 86509.Jan 31 2017, 3:33 PM

Fixing the issue labath reported in the review. Generally speaking we shouldn't be configuring or compiling plugins that can't be used.

The main code seems to be fine now, but linking of unit tests (check-lldb-unit target) fails now. You'll probably need to explicitly set their dependencies as well before this can be removed.

beanz added a comment.Feb 1 2017, 11:32 AM

Ah! I forgot to update the unittest directory. I'll get a patch in today that updates the unittests following the pattern from the libraries.

beanz updated this revision to Diff 86721.Feb 1 2017, 2:30 PM

Unit tests are updated in rL293821. This update makes one last change to the unit tests to build with the explicit dependencies.

labath added a comment.Feb 1 2017, 4:24 PM

Unfortunately, I am still getting undefined symbols when building some of the unittest binaries:

FAILED: : && /usr/bin/clang++-3.6   -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -Werror=date-time -std=c++11 -fcolor-diagnostics -ffunction-sections -fdata-sections -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3 -DNDEBUG -ffunction-sections -fdata-sections -fuse-ld=gold  -Wl,-allow-shlib-undefined -Wl,--icf=safe   -Wl,-O3 -Wl,--gc-sections tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o  -o tools/lldb/unittests/Breakpoint/LLDBBreakpointTests  lib/libLLVMSupport.a lib/libLLVMSupport.a -lpthread lib/libgtest_main.a lib/libgtest.a -lpthread -ledit -lcurses /usr/lib/x86_64-linux-gnu/libform.so /usr/lib/x86_64-linux-gnu/libpanel.so -ltinfo /usr/lib/x86_64-linux-gnu/libpython2.7.so -lpthread -ldl -lcurses /usr/lib/x86_64-linux-gnu/libform.so /usr/lib/x86_64-linux-gnu/libpanel.so lib/libLLVMSupport.a -lrt -latomic -lpthread -lz -lm lib/libLLVMDemangle.a /usr/lib/x86_64-linux-gnu/libpython2.7.so -ltinfo -lpthread -ldl && cd /usr/local/google/home/labath/ll/build/opt/tools/lldb/unittests/Breakpoint && /usr/local/google/home/labath/local/cmake/bin/cmake -E make_directory /usr/local/google/home/labath/ll/build/opt/tools/lldb/unittests/Breakpoint/Inputs
tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o:/usr/local/google/home/labath/ll/llvm/tools/lldb/unittests/Breakpoint/BreakpointIDTest.cpp:function BreakpointIDTest_StringIsBreakpointName_Test::TestBody(): error: undefined reference to 'lldb_private::Error::Error()'
tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o:/usr/local/google/home/labath/ll/llvm/tools/lldb/unittests/Breakpoint/BreakpointIDTest.cpp:function BreakpointIDTest_StringIsBreakpointName_Test::TestBody(): error: undefined reference to 'lldb_private::BreakpointID::StringIsBreakpointName(llvm::StringRef, lldb_private::Error&)'
tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o:/usr/local/google/home/labath/ll/llvm/tools/lldb/unittests/Breakpoint/BreakpointIDTest.cpp:function BreakpointIDTest_StringIsBreakpointName_Test::TestBody(): error: undefined reference to 'lldb_private::BreakpointID::StringIsBreakpointName(llvm::StringRef, lldb_private::Error&)'
tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o:/usr/local/google/home/labath/ll/llvm/tools/lldb/unittests/Breakpoint/BreakpointIDTest.cpp:function BreakpointIDTest_StringIsBreakpointName_Test::TestBody(): error: undefined reference to 'lldb_private::BreakpointID::StringIsBreakpointName(llvm::StringRef, lldb_private::Error&)'
tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o:/usr/local/google/home/labath/ll/llvm/tools/lldb/unittests/Breakpoint/BreakpointIDTest.cpp:function BreakpointIDTest_StringIsBreakpointName_Test::TestBody(): error: undefined reference to 'lldb_private::BreakpointID::StringIsBreakpointName(llvm::StringRef, lldb_private::Error&)'
tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o:/usr/local/google/home/labath/ll/llvm/tools/lldb/unittests/Breakpoint/BreakpointIDTest.cpp:function BreakpointIDTest_StringIsBreakpointName_Test::TestBody(): error: undefined reference to 'lldb_private::Error::~Error()'
tools/lldb/unittests/Breakpoint/CMakeFiles/LLDBBreakpointTests.dir/BreakpointIDTest.cpp.o:/usr/local/google/home/labath/ll/llvm/tools/lldb/unittests/Breakpoint/BreakpointIDTest.cpp:function BreakpointIDTest_StringIsBreakpointName_Test::TestBody(): error: undefined reference to 'lldb_private::Error::~Error()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This one is particularly strange as the breakpoint tests explicitly specify they depend in LldbBreakpoint, but I don't see them anywhere on the link line. I haven't yet tried to dig further into what is going on. Do you have a linux box you can try this out on?

labath added a comment.Feb 1 2017, 5:21 PM

BTW, I just noticed that the Breakpoint change has dos line breaks for some reason. Maybe that could be the cause. I'll try it out tomorrow.

labath added a comment.Feb 1 2017, 5:48 PM

the newlines were a red herring. I think the problem is that you forgot to use the LINK_LIBS argument (see comment).

This works on linux after applying this change (and fixing ObjectFileELF test dependencies, which you missed because they were added yesterday).

unittests/CMakeLists.txt
42 ↗(On Diff #86721)

add ${ARG_LINK_LIBS} here

beanz updated this revision to Diff 87265.Feb 6 2017, 10:50 AM

Rebasing patch

labath accepted this revision.Feb 7 2017, 9:13 AM

Looks great.

This revision is now accepted and ready to land.Feb 7 2017, 9:13 AM
beanz added a comment.Feb 7 2017, 3:48 PM

Ok. I think this patch is fully ready to go now. I added the missing dependencies in the ObjectFileELF tests in rL294372.

Thank you @labath for all your patience and help testing this patch. I've tested it on Darwin and FreeBSD in the current incarnation. I'm going to try and land it tomorrow, and I will resolve any issues the bots encounter.

This revision was automatically updated to reflect the committed changes.