Page MenuHomePhabricator

[cmake] Move LLVM_TOUCH_STATIC_LIBRARIES logic to llvm_update_compile_flags
Needs ReviewPublic

Authored by hintonda on May 25 2019, 12:17 PM.

Details

Reviewers
beanz
Summary

This is a refinement of D62172, r361280, and moves the
LLVM_TOUCH_STATIC_LIBRARIES logic to llvm_update_compile_flags so it
can be used by static libraries created with add_library and not just
those created with llvm_add_library.

Event Timeline

hintonda created this revision.May 25 2019, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2019, 12:17 PM
Herald added a subscriber: mgorny. · View Herald Transcript

@beanz, could you take a look? thanks...

beanz added a comment.Mon, Jul 8, 1:20 PM

I don't like the idea of putting this in llvm_update_compile_flags. That function does exactly what its name says, and adding additional behavior to it seems undesirable. Where are we creating libraries with add_library that we need this?

One thought would be to make a new CMake function to apply the workaround.

I don't like the idea of putting this in llvm_update_compile_flags. That function does exactly what its name says, and adding additional behavior to it seems undesirable. Where are we creating libraries with add_library that we need this?

There might be others, but these are the ones I always see:

$ find llvm -type f \( -name "*.cmake" -o -name "*.txt" \) -exec grep -Hn llvm_update_compile_flags \{\} \;
llvm/tools/llvm-cfi-verify/lib/CMakeLists.txt:9:llvm_update_compile_flags(LLVMCFIVerify)
llvm/tools/llvm-exegesis/lib/CMakeLists.txt:38:llvm_update_compile_flags(LLVMExegesis)
llvm/tools/llvm-exegesis/lib/PowerPC/CMakeLists.txt:11:llvm_update_compile_flags(LLVMExegesisPowerPC)
llvm/tools/llvm-exegesis/lib/X86/CMakeLists.txt:11:llvm_update_compile_flags(LLVMExegesisX86)
llvm/tools/llvm-exegesis/lib/AArch64/CMakeLists.txt:11:llvm_update_compile_flags(LLVMExegesisAArch64)

One thought would be to make a new CMake function to apply the workaround.

Okay, I'll see if I can come up with something as clean as this.

beanz added a comment.Tue, Jul 9, 9:49 AM

One thought, we should probably evaluate why those tools are using add_library instead of add_llvm_library. The right fix might be to move them over. This is exactly the kind of behavior that add_llvm_library and llvm_add_library were designed to handle.

One thought, we should probably evaluate why those tools are using add_library instead of add_llvm_library. The right fix might be to move them over. This is exactly the kind of behavior that add_llvm_library and llvm_add_library were designed to handle.

I agree, however, I remember a lot of churn when these were added, and didn't really follow why they needed to be different, so I was hesitant to make any gratuitous changes. My goal was just to have nice clean builds on my laptop that didn't take too long or report spurious errors.

I'll take another look and see if I can figure out why they didn't work out of the box.