This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Unbreak LLVM-Config.cmake / llvm_expand_dependencies
ClosedPublic

Authored by whitequark on Nov 3 2014, 2:03 PM.

Details

Reviewers
chandlerc
rnk
Summary

The algorithm for sorting libraries in topological order, as
previously implemented, had a few issues:

  • It didn't make any sense.
  • It didn't actually sort libraries in topological order.
  • It hung on some inputs, e.g. "LLVMipo".

This commit replaces the old algorithm with a straightforward port
from llvm-config.cpp.

Diff Detail

Event Timeline

whitequark updated this revision to Diff 15731.Nov 3 2014, 2:03 PM
whitequark retitled this revision from to [cmake] Unbreak LLVM-Config.cmake / llvm_expand_dependencies.
whitequark updated this object.
whitequark edited the test plan for this revision. (Show Details)
whitequark added a reviewer: chandlerc.
whitequark set the repository for this revision to rL LLVM.
whitequark added a subscriber: Unknown Object (MLST).

The new implementation was tested in the CMake buildsystem of the OCaml bindings, which I will post a bit later. It links a lot of libraries in different combinations, which, with BUILD_SHARED_LIBS=OFF, exposed both the incorrect ordering and the hangups in the old implementation. The new one works perfectly well.

rnk added a subscriber: rnk.Dec 1 2014, 12:38 PM
rnk added inline comments.
cmake/modules/LLVM-Config.cmake
155

Can you add a comment before the function describing what it does, and perhaps reference the llvm-config code? It's doing a post-order traversal, which is apparently not what the linker expects.

Would a pre-order traversal save the list reversal? It's not immediately clear to me that it would.

183

I guess the linker actually expects roots first and leaves last.

whitequark added inline comments.Dec 1 2014, 12:45 PM
cmake/modules/LLVM-Config.cmake
155

It just duplicates the algorithm in the llvm-config binary to keep things simple. Also IIRC I started with a pre-order traversal and it looked really ugly when implemented in cmake.

183

What do you mean? llvm_expand_dependencies currently uses the correct ordering; it won't build without this REVERSE.

whitequark updated this revision to Diff 17475.Dec 18 2014, 3:55 PM
whitequark set the repository for this revision to rL LLVM.

@rnk, I've added the documentation.

rnk accepted this revision.Dec 18 2014, 3:56 PM
rnk added a reviewer: rnk.

lgtm

Thanks!

This revision is now accepted and ready to land.Dec 18 2014, 3:56 PM