Page MenuHomePhabricator

[CMake] Add accurate dependency specifications
ClosedPublic

Authored by beanz on Jan 31 2017, 9:27 AM.

Details

Summary

This patch adds accurate dependency specifications to the mail LLDB libraries and tools.

In all cases except lldb-server, these dependencies are added in addition to existing dependencies (making this low risk), and I performed some code cleanup along the way.

For lldb-server I've cleaned up the LLVM dependencies down to just the minimum actually required. This is more than lldb-server actually directly references, and I've left a todo in the code to clean that up.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz created this revision.Jan 31 2017, 9:27 AM
beanz added a subscriber: lldb-commits.
labath edited edge metadata.Jan 31 2017, 10:26 AM

I like where this is going. I am not sure about a couple of details though. Please see comments.

source/Breakpoint/CMakeLists.txt
32 ↗(On Diff #86446)

Shouldn't this be: LINK_COMPONENTS Support? I have a feeling that if you specify it this way, it will break LLVM_LINK_LLVM_DYLIB build.

tools/lldb-server/CMakeLists.txt
63 ↗(On Diff #86446)

This seems wrong. lldb-server does not link to liblldb at all, so it's link line should not be affected by the LLDB_EXPORT_ALL_SYMBOLS setting. lldb-server links agains the .a files directly.

beanz updated this revision to Diff 86461.Jan 31 2017, 10:43 AM

Updates based on post commit feedback from labath.

He was right on both points. LLVM libs should be done through LINK_COMPONENTS, and I messed up lldb-server's dependencies.

zturner edited edge metadata.Jan 31 2017, 10:49 AM

Any reason this doesn't use the same strategy as LLVM? I.e. at the top of the CMakeLists.txt file, write something like set(LLVM_LINK_COMPONENTS Foo Bar Baz), then just call add_lldb_library etc.

@zturner That mechanism does work, however I generally find that it is cleaner to pass named parameters into CMake functions which provides a bit of a self-documenting API, as opposed to relying on known named variables. The named-parameter was introduced for LLD, and I think is the more-modern way of handling this. Eventually I think LLVM should be updated to do the same.

As long as you considered the tradeoffs then and you like this better, that's fine then.

Is CMake going to complain about circular dependencies? For example, Breakpoint depends on Core and Core depends on Breakpoint.

Thankfully CMake will not complain about circular dependencies in static archive targets. If it did, we'd really be in trouble because the LLDB dependencies graph has *lots* of circular dependencies. Actually, I think CMake even handles them properly on Linux, which should eliminate our need to add --start-group and --end-group to the linker command lines.

zturner accepted this revision.Jan 31 2017, 11:14 AM

ok, lgtm with the caveat that if it breaks anything on Windows we might have to revert until we figure it out.

This revision is now accepted and ready to land.Jan 31 2017, 11:14 AM

@zturner, absolutely! Thanks!

I expect this patch and the next one to be pretty safe because I'm not actually removing the existing dependency specifications, just adding new explicit ones.

labath accepted this revision.Jan 31 2017, 11:22 AM

Thankfully CMake will not complain about circular dependencies in static archive targets. If it did, we'd really be in trouble because the LLDB dependencies graph has *lots* of circular dependencies. Actually, I think CMake even handles them properly on Linux, which should eliminate our need to add --start-group and --end-group to the linker command lines.

I was thinking about that as well. I am not sure if it will work if we actually need multiple iterations of the loop to get all the dependencies converging, but it may be worth trying out.

I am hoping we will be able to reduce the loops in the future. My plan is that after finishing with the Log class, to move it to a new module, with clean and well defined dependencies (I need to remove all ConstStrings and LLDB Streams from it first), After that I want start moving other low level functionalities there as well. That got a bit delayed now, but I should be back to that in a week or two.

I was thinking about that as well. I am not sure if it will work if we actually need multiple iterations of the loop to get all the dependencies converging, but it may be worth trying out.

The way it is supposed to work, CMake will duplicate the libraries on the command line in the right order to avoid the need for looping at all.

I am hoping we will be able to reduce the loops in the future. My plan is that after finishing with the Log class, to move it to a new module, with clean and well defined dependencies (I need to remove all ConstStrings and LLDB Streams from it first), After that I want start moving other low level functionalities there as well. That got a bit delayed now, but I should be back to that in a week or two.

This would be great!

I was thinking about that as well. I am not sure if it will work if we actually need multiple iterations of the loop to get all the dependencies converging, but it may be worth trying out.

The way it is supposed to work, CMake will duplicate the libraries on the command line in the right order to avoid the need for looping at all.

I am not sure that is enough. Imagine this:
X.a(x1.o x2.o)
Y.a(y1.o y2.o)

if X depends on Y and vice-versa, cmake will add something like -lX -lY -lX
however, if the dependency graph is something like:
main.o -> x1.o -> y1.o -> x2.o -> y2.o

then this won't be enough because by the time the linker figures out it really needs y2.o it will already have scanned past the -lY, and the link will still fail. It this example it is enough to repeat all libraries twice, but in theory the chain can be arbitrarily long and cmake has no way of figuring that out.

labath4 /tmp/X $ nm x.a

x1.o:
0000000000000000 T _x1
                 U _y1

x2.o:
0000000000000000 T _x2
                 U _y2
labath4 /tmp/X $ nm y.a

y1.o:
                 U _x2
0000000000000000 T _y1

y2.o:
0000000000000000 T _y2

labath4 /tmp/X $ gcc main.c
/tmp/ccAoBLxC.o: In function `main':
main.c:(.text+0xa): undefined reference to `_x1'
collect2: error: ld returned 1 exit status

labath4 /tmp/X $ gcc main.c x.a y.a x.a
x.a(x2.o): In function `_x2':
a.c:(.text+0xa): undefined reference to `_y2'
collect2: error: ld returned 1 exit status

labath4 /tmp/X $ gcc main.c x.a y.a x.a y.a && echo OK
OK

That said, maybe the situation in lldb is not so dire, and it will actually work -- only one way to find out.

@labath, we'll have to see if CMake's handling is good enough. What CMake actually does is repeat the full chain of the cycle, so it would be something like adding -lX -lY -lX -lY in your example. This doesn't work for certain pathological cases, but CMake actually has mechanisms to help deal with that too if we need them:

https://cmake.org/cmake/help/v3.4/command/target_link_libraries.html#cyclic-dependencies-of-static-libraries

This revision was automatically updated to reflect the committed changes.