This is an archive of the discontinued LLVM Phabricator instance.

Statically link llvm-cfi-verify's libraries.
ClosedPublic

Authored by hctim on Oct 17 2017, 2:15 PM.

Details

Summary

llvm-cfi-verify (D38379) introduced a potential build failure when compiling with -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON. Specific versions of cmake seem to treat the add_subdirectory() rule differently. It seems as if old versions of cmake BFS these rules, adding them to the fringe for expansion later. Newer versions of cmake seem to immediately execute CMakeFiles that are present in this subdirectory.

If the subdirectory is expanded through the fringe, the globbing resultant from llvm_add_implicit_projects() from cmake/modules/AddLLVM.cmake:1012 means that tools/llvm-shlib/CMakeFile.txt gets executed before tools/llvm-cfi-verify/lib/CMakeFile.txt. As the latter CMakeFile adds a new library, this expansion order means that the library files required the unit tests in unittests/tools/llvm-cfi-verify/ are not present in the dynamic library. This causes unit tests to fail as the required functions can't be found.

This change now ensures that the libraries created by llvm-cfi-verify are statically linked into the unit tests. As tools/llvm-cfi-verify/lib no longer adds anything to llvm-shlib, there should be no concern about the order-of-compilation.

Event Timeline

hctim created this revision.Oct 17 2017, 2:15 PM
pcc added a comment.Oct 17 2017, 2:40 PM

Taking a step back, I don't think we even want the llvm-cfi-verify code in the shared library. That is because it is only intended to be used by one executable and the test suite. If you can avoid linking it in to the shlib, I guess you wouldn't need to do anything about the enumeration order of llvm-shlib.

Does it not work to declare your library with add_library instead of add_llvm_library? Alternatively it looks like add_llvm_library has a BUILDTREE_ONLY flag that appears to do what is needed here.

hctim added a comment.Oct 17 2017, 3:05 PM
In D39020#900124, @pcc wrote:

Does it not work to declare your library with add_library instead of add_llvm_library? Alternatively it looks like add_llvm_library has a BUILDTREE_ONLY flag that appears to do what is needed here.

Adding BUILDTREE_ONLY both with and without STATIC are resulting in the same errors. Will take a look at explicitly creating the library without the llvm helpers through add_library.

hctim updated this revision to Diff 119404.Oct 17 2017, 4:59 PM

Explicitly made llvm-cfi-verify build its own library that is statically linked into CFIVerifyTests. The target is not linked into the dynamic library when LLVM_BUILD_LLVM_DYLIB is enabled.

hctim retitled this revision from Made shlib compile explicitly AFTER all tools. to Statically link llvm-cfi-verify's libraries..Oct 17 2017, 5:01 PM
hctim edited the summary of this revision. (Show Details)
pcc added inline comments.Oct 17 2017, 5:33 PM
tools/llvm-cfi-verify/lib/CMakeLists.txt
19

Can you move the list of dependencies into this statement?

unittests/tools/llvm-cfi-verify/CMakeLists.txt
18

Why is this line necessary?

hctim updated this revision to Diff 119411.Oct 17 2017, 5:48 PM
hctim marked 2 inline comments as done.
  • pcc@'s comments
unittests/tools/llvm-cfi-verify/CMakeLists.txt
18

It isn't. Removed.

pcc accepted this revision.Oct 17 2017, 5:53 PM

LGTM

tools/llvm-cfi-verify/lib/CMakeLists.txt
1

I think you want to depend on this library from llvm-cfi-verify instead of building it twice.

This revision is now accepted and ready to land.Oct 17 2017, 5:53 PM
skatkov accepted this revision.Oct 17 2017, 9:17 PM

Confirm, the patch fixes my issue.

Please land it asap.

This revision was automatically updated to reflect the committed changes.