This is an archive of the discontinued LLVM Phabricator instance.

[test-suite] Append LDFLAGS to LINK_LIBRARIES
ClosedPublic

Authored by mcrosier on Mar 9 2016, 12:01 PM.

Details

Summary

r261397 changed how LDFLAGS were added to the target under cmake from
using target_link_libraries to using a function to call
append_target_flags to append LDFLAGS to LINK_FLAGS because
target_link_libraries ignores flags that do not begin with -; however,
LINK_FLAGS are appended before the object files on the link line, so
this causes linking statically to fail.

This patches changes the append_compile_flags macro to append LDFLAGS to
LINK_LIBRARIES instead of LINK_FLAGS, as LINK_LIBRARIES is appended
after the object files. In addition, it modifies the append_target_flags
macro to strip whitespace before setting the destination variable, as
having any whitespace before or after LINK_LIBRARIES violates CMP0004.

Diff Detail

Event Timeline

mcrosier updated this revision to Diff 50174.Mar 9 2016, 12:01 PM
mcrosier retitled this revision from to [test-suite] Use target_link_libraries for libs.
mcrosier updated this object.
mcrosier added reviewers: MatzeB, jmolloy, kristof.beyls.
mcrosier added a subscriber: llvm-commits.
MatzeB edited edge metadata.Mar 9 2016, 1:01 PM

Have you tried what happens when we change the append_link_flags macro to:

macro(append_link_flags target)
  append_target_flags(LINK_LIBRARIES ${target} ${ARGN})
endmacro()

My understanding from the cmake docu is that those get appended after the object files, what is not clear to me from the docu is whether we get the same stupid behavior as target_link_libraries() but I think there is a chance that the target property is just appended without cmake trying to be smart...

mcrosier updated this revision to Diff 50339.Mar 10 2016, 1:05 PM
mcrosier retitled this revision from [test-suite] Use target_link_libraries for libs to [test-suite] Append LDFLAGS to LINK_LIBRARIES.
mcrosier updated this object.
mcrosier edited edge metadata.

Update based on Matthias' comments.

MatzeB accepted this revision.Mar 10 2016, 2:37 PM
MatzeB edited edge metadata.

Tried this and it works fine for me (the -Xlinker 0x800000 that made switch away from target_link_library() is still passed correctly to the linker).
LGTM.

cmake/modules/SingleMultiSource.cmake
243

accidental newline?

This revision is now accepted and ready to land.Mar 10 2016, 2:37 PM

Committed r263235. Thanks, Matthias.

mcrosier closed this revision.Mar 11 2016, 6:14 AM