This is an archive of the discontinued LLVM Phabricator instance.

Differential D154212

[cmake] Add LLVM_UNITTEST_LINK_FLAGS option
ClosedPublic

Authored by nikic on Jun 30 2023, 6:07 AM.

Details

Reviewers
mgorny
phosek
Ericson2314
Commits
rGc0011f027d68: [cmake] Add LLVM_UNITTEST_LINK_FLAGS option
Summary

Add an option to specify additional linker flags for unit tests only. For example, this allows doing something like -DLLVM_UNITTEST_LINK_FLAGS="-Wl,-plugin-opt=O0" if you're doing LTO builds, or -DLLVM_UNITTEST_LINK_FLAGS="-fno-lto" if you're using fat LTO objects.

The build system already does this itself if the LLVM_ENABLE_LTO flag is used, but this does not cover all possible LTO configurations. (E.g. we're using ld.bfd with LLVMgold.so, which isn't one of the supported configurations.)

Diff Detail

Unit TestsFailed

TimeTest
530 msx64 debian > LLVM.CodeGen/ARM::llvm.frexp.ll
480 msx64 debian > LLVM.CodeGen/PowerPC::llvm.frexp.ll
490 msx64 debian > LLVM.CodeGen/X86::llvm.frexp.f80.ll
490 msx64 debian > LLVM.CodeGen/X86::llvm.frexp.ll
60,060 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp

Event Timeline

nikic created this revision.Jun 30 2023, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2023, 6:07 AM
Herald added subscribers: ekilmer, StephenFan. · View Herald Transcript
nikic requested review of this revision.Jun 30 2023, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2023, 6:07 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B242391: Diff 536201.Jun 30 2023, 6:42 AM
phosek added inline comments.Jun 30 2023, 9:37 AM
llvm/cmake/modules/AddLLVM.cmake
1613–1614

For variables that are intended to be used by users, I'd prefer to keep them in the top-level CMakeLists.txt file for easier discoverability.

1615–1616

LINK_FLAGS property is deprecated and superseded by LINK_OPTIONS, see https://cmake.org/cmake/help/latest/prop_tgt/LINK_FLAGS.html. Can you use target_link_options instead?

nikic added inline comments.Jun 30 2023, 2:21 PM
llvm/cmake/modules/AddLLVM.cmake
1613–1614

The reason I put it here is that the option is shared across all subprojects using unit tests, so just putting it in llvm/CMakeLists.txt wouldn't work. Maybe HandleLLVMOptions would be the right place to define such a shared option?

nikic updated this revision to Diff 536711.Jul 3 2023, 3:39 AM

Move option to HandleLLVMOptions, use target_link_options.

Harbormaster completed remote builds in B242762: Diff 536711.Jul 3 2023, 4:29 AM
phosek accepted this revision.Jul 4 2023, 7:32 PM

LGTM

This revision is now accepted and ready to land.Jul 4 2023, 7:32 PM
Closed by commit rGc0011f027d68: [cmake] Add LLVM_UNITTEST_LINK_FLAGS option (authored by nikic). · Explain WhyJul 5 2023, 12:19 AM
This revision was automatically updated to reflect the committed changes.
nikic added a commit: rGc0011f027d68: [cmake] Add LLVM_UNITTEST_LINK_FLAGS option.

Revision Contents

PathSize
llvm/
cmake/
modules/
5 lines

Diff 536201

llvm/cmake/modules/AddLLVM.cmake

Show First 20 Lines • Show All 1,604 Lines • ▼ Show 20 Lines elseif(LINKER_IS_LLD_LINK)
set_property(TARGET ${test_name} APPEND_STRING PROPERTY set_property(TARGET ${test_name} APPEND_STRING PROPERTY
LINK_FLAGS " /opt:lldlto=0") LINK_FLAGS " /opt:lldlto=0")
elseif(APPLE AND NOT uppercase_LLVM_ENABLE_LTO STREQUAL "THIN") elseif(APPLE AND NOT uppercase_LLVM_ENABLE_LTO STREQUAL "THIN")
set_property(TARGET ${target_name} APPEND_STRING PROPERTY set_property(TARGET ${target_name} APPEND_STRING PROPERTY
LINK_FLAGS " -Wl,-mllvm,-O0") LINK_FLAGS " -Wl,-mllvm,-O0")
endif() endif()
endif() endif()
set(LLVM_UNITTEST_LINK_FLAGS "" CACHE STRING
"Additional linker flags for unit tests")
phosekUnsubmitted
Not Done
ReplyInline Actions

For variables that are intended to be used by users, I'd prefer to keep them in the top-level CMakeLists.txt file for easier discoverability.

phosek: For variables that are intended to be used by users, I'd prefer to keep them in the top-level…
nikicAuthorUnsubmitted
Done
ReplyInline Actions

The reason I put it here is that the option is shared across all subprojects using unit tests, so just putting it in llvm/CMakeLists.txt wouldn't work. Maybe HandleLLVMOptions would be the right place to define such a shared option?

nikic: The reason I put it here is that the option is shared across all subprojects using unit tests…
set_property(TARGET ${test_name} APPEND_STRING PROPERTY
LINK_FLAGS " ${LLVM_UNITTEST_LINK_FLAGS}")
phosekUnsubmitted
Not Done
ReplyInline Actions

LINK_FLAGS property is deprecated and superseded by LINK_OPTIONS, see https://cmake.org/cmake/help/latest/prop_tgt/LINK_FLAGS.html. Can you use target_link_options instead?

phosek: `LINK_FLAGS` property is deprecated and superseded by `LINK_OPTIONS`, see https://cmake.
set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR}) set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir}) set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
# libpthreads overrides some standard library symbols, so main # libpthreads overrides some standard library symbols, so main
# executable must be linked with it in order to provide consistent # executable must be linked with it in order to provide consistent
# API for all shared libaries loaded by this executable. # API for all shared libaries loaded by this executable.
target_link_libraries(${test_name} PRIVATE llvm_gtest_main llvm_gtest ${LLVM_PTHREAD_LIB}) target_link_libraries(${test_name} PRIVATE llvm_gtest_main llvm_gtest ${LLVM_PTHREAD_LIB})
add_dependencies(${test_suite} ${test_name}) add_dependencies(${test_suite} ${test_name})
▲ Show 20 Lines • Show All 861 Lines • Show Last 20 Lines