Differential D29333
[CMake] Add accurate dependency specifications Authored by beanz on Jan 31 2017, 9:27 AM.
Details 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
Event TimelineComment Actions I like where this is going. I am not sure about a couple of details though. Please see comments.
Comment Actions 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. Comment Actions 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. Comment Actions @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. Comment Actions 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. Comment Actions 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. Comment Actions ok, lgtm with the caveat that if it breaks anything on Windows we might have to revert until we figure it out. Comment Actions @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. Comment Actions 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. Comment Actions 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.
This would be great! Comment Actions I am not sure that is enough. Imagine this: if X depends on Y and vice-versa, cmake will add something like -lX -lY -lX 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
OKThat said, maybe the situation in lldb is not so dire, and it will actually work -- only one way to find out. Comment Actions @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: | ||||||||||||