This is an archive of the discontinued LLVM Phabricator instance.

Differential D115276

[CMake] Move the AIX archiver settings to a module
ClosedPublic

Authored by phosek on Dec 7 2021, 12:17 PM.

Details

Reviewers
beanz
smeenai
kaz7
ldionne
Commits
rG96e36048fddf: [CMake] Move the AIX archiver settings to a module
Summary

This allows their reuse across projects. The name of the module
is intentionally generic because we would like to move more platform
checks there.

Diff Detail

Event Timeline

phosek created this revision.Dec 7 2021, 12:17 PM
Herald added a subscriber: mgorny. · View Herald TranscriptDec 7 2021, 12:17 PM
phosek requested review of this revision.Dec 7 2021, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 12:17 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
phosek added a reviewer: ldionne.Dec 7 2021, 12:17 PM
phosek added a comment.Dec 7 2021, 12:21 PM

I'm thinking about moving https://github.com/llvm/llvm-project/blob/cfe5d768be95ae0a62cf430e56e7762cebf81a65/llvm/cmake/modules/HandleLLVMOptions.cmake#L174 and https://github.com/llvm/llvm-project/blob/cfe5d768be95ae0a62cf430e56e7762cebf81a65/compiler-rt/cmake/Modules/UseLibtool.cmake into CheckPlatform.cmake since this is logic we want to reuse across projects. Let me know if you have a better suggestion for the name of this module.

Harbormaster completed remote builds in B137971: Diff 392498.Dec 7 2021, 1:06 PM
ldionne added a comment.Dec 7 2021, 2:59 PM
In D115276#3177373, @phosek wrote:

Let me know if you have a better suggestion for the name of this module.

SetArchiverForPlatform? Or something that conveys the fact that (1) this is basically about ar, and (2) it actually performs an action (it sets global variables).

ldionne added a comment.Dec 7 2021, 3:00 PM
In D115276#3177795, @ldionne wrote:
In D115276#3177373, @phosek wrote:

Let me know if you have a better suggestion for the name of this module.

SetArchiverForPlatform? Or something that conveys the fact that (1) this is basically about ar, and (2) it actually performs an action (it sets global variables).

SetPlatformToolchainTools?

kaz7 added a comment.Dec 7 2021, 4:07 PM

+1 for SetPlatformToolchainTools. :-)

lkail added a subscriber: lkail.Dec 7 2021, 6:04 PM
simoll added a subscriber: simoll.Dec 9 2021, 4:45 AM
kaz7 added a comment.Dec 13 2021, 10:21 PM

Is it OK to accept this patch by me? Or should we wait for @ldionne? Just curious.

ldionne added a comment.Dec 14 2021, 6:27 AM
In D115276#3191287, @kaz7 wrote:

Is it OK to accept this patch by me? Or should we wait for @ldionne? Just curious.

For the record, the review guidelines we have for libc++/libc++abi do not necessarily apply to compiler-rt, so there's no requirement that someone from the libc++ review group gives an approval for this to make progress.

kaz7 added a comment.Dec 14 2021, 7:17 AM
In D115276#3192146, @ldionne wrote:
In D115276#3191287, @kaz7 wrote:

Is it OK to accept this patch by me? Or should we wait for @ldionne? Just curious.

For the record, the review guidelines we have for libc++/libc++abi do not necessarily apply to compiler-rt, so there's no requirement that someone from the libc++ review group gives an approval for this to make progress.

Thank you for clarifying.

phosek updated this revision to Diff 397645.Jan 5 2022, 11:18 AM

Renamed the module to SetPlatformToolchainTools.cmake as suggested.

smeenai accepted this revision.Jan 5 2022, 11:35 AM

LGTM

cmake/Modules/SetPlatformToolchainTools.cmake
3 ↗(On Diff #397645)

Super nit: using an unquoted ${} inside an if is a bad idea cos of https://cmake.org/cmake/help/latest/command/if.html#variable-expansion. Should be either if(CMAKE_SYSTEM_NAME or if("${CMAKE_SYSTEM_NAME}"

This revision is now accepted and ready to land.Jan 5 2022, 11:35 AM
smeenai added inline comments.Jan 5 2022, 11:36 AM
cmake/Modules/SetPlatformToolchainTools.cmake
3 ↗(On Diff #397645)

(I know it's just a copypasta of the existing code, but might as well fix it while we're here)

phosek updated this revision to Diff 397652.Jan 5 2022, 11:46 AM
phosek marked 2 inline comments as done.
This revision was landed with ongoing or failed builds.Jan 5 2022, 11:46 AM
Closed by commit rG96e36048fddf: [CMake] Move the AIX archiver settings to a module (authored by phosek). · Explain Why
This revision was automatically updated to reflect the committed changes.
phosek added a commit: rG96e36048fddf: [CMake] Move the AIX archiver settings to a module.
Harbormaster completed remote builds in B141728: Diff 397652.Jan 5 2022, 11:46 AM

Revision Contents

PathSize
cmake/
Modules/
9 lines
compiler-rt/
10 lines
lib/
builtins/
10 lines

Diff 392498

cmake/Modules/CheckPlatform.cmake

  • This file was added.
get_property(languages GLOBAL PROPERTY ENABLED_LANGUAGES)
if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
foreach(lang ${languages})
set(CMAKE_${lang}_ARCHIVE_CREATE "<CMAKE_AR> -X32_64 qc <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_${lang}_ARCHIVE_APPEND "<CMAKE_AR> -X32_64 q <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_${lang}_ARCHIVE_FINISH "<CMAKE_RANLIB> -X32_64 <TARGET>")
endforeach()
endif()

compiler-rt/CMakeLists.txt

Show All 20 Lineslist(INSERT CMAKE_MODULE_PATH 0
) )
if(CMAKE_CONFIGURATION_TYPES) if(CMAKE_CONFIGURATION_TYPES)
set(CMAKE_CFG_RESOLVED_INTDIR "${CMAKE_CFG_INTDIR}/") set(CMAKE_CFG_RESOLVED_INTDIR "${CMAKE_CFG_INTDIR}/")
else() else()
set(CMAKE_CFG_RESOLVED_INTDIR "") set(CMAKE_CFG_RESOLVED_INTDIR "")
endif() endif()
include(CheckPlatform)
include(base-config-ix) include(base-config-ix)
include(CompilerRTUtils) include(CompilerRTUtils)
option(COMPILER_RT_BUILD_BUILTINS "Build builtins" ON) option(COMPILER_RT_BUILD_BUILTINS "Build builtins" ON)
mark_as_advanced(COMPILER_RT_BUILD_BUILTINS) mark_as_advanced(COMPILER_RT_BUILD_BUILTINS)
option(COMPILER_RT_BUILD_CRT "Build crtbegin.o/crtend.o" ON) option(COMPILER_RT_BUILD_CRT "Build crtbegin.o/crtend.o" ON)
mark_as_advanced(COMPILER_RT_BUILD_CRT) mark_as_advanced(COMPILER_RT_BUILD_CRT)
option(COMPILER_RT_CRT_USE_EH_FRAME_REGISTRY "Use eh_frame in crtbegin.o/crtend.o" ON) option(COMPILER_RT_CRT_USE_EH_FRAME_REGISTRY "Use eh_frame in crtbegin.o/crtend.o" ON)
▲ Show 20 Lines • Show All 549 Lines • ▼ Show 20 Linesif (CMAKE_LINKER MATCHES "link.exe$")
# files, rather than skipping these files in the build system. # files, rather than skipping these files in the build system.
# Ideally, we would pass this flag only for the libraries that need # Ideally, we would pass this flag only for the libraries that need
# it, but CMake doesn't seem to have a way to set linker flags for # it, but CMake doesn't seem to have a way to set linker flags for
# individual static libraries, so we enable the suppression flag for # individual static libraries, so we enable the suppression flag for
# the whole compiler-rt project. # the whole compiler-rt project.
set(CMAKE_STATIC_LINKER_FLAGS "${CMAKE_STATIC_LINKER_FLAGS} /IGNORE:4221") set(CMAKE_STATIC_LINKER_FLAGS "${CMAKE_STATIC_LINKER_FLAGS} /IGNORE:4221")
endif() endif()
if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
set(CMAKE_C_ARCHIVE_CREATE "<CMAKE_AR> -X32_64 qc <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_CXX_ARCHIVE_CREATE "<CMAKE_AR> -X32_64 qc <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_C_ARCHIVE_APPEND "<CMAKE_AR> -X32_64 q <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_CXX_ARCHIVE_APPEND "<CMAKE_AR> -X32_64 q <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_C_ARCHIVE_FINISH "<CMAKE_RANLIB> -X32_64 <TARGET>")
set(CMAKE_CXX_ARCHIVE_FINISH "<CMAKE_RANLIB> -X32_64 <TARGET>")
endif()
add_subdirectory(include) add_subdirectory(include)
option(COMPILER_RT_USE_LIBCXX option(COMPILER_RT_USE_LIBCXX
"Enable compiler-rt to use libc++ from the source tree" ON) "Enable compiler-rt to use libc++ from the source tree" ON)
if(COMPILER_RT_USE_LIBCXX) if(COMPILER_RT_USE_LIBCXX)
if(LLVM_ENABLE_PROJECTS_USED) if(LLVM_ENABLE_PROJECTS_USED)
# Don't use libcxx if LLVM_ENABLE_PROJECTS does not enable it. # Don't use libcxx if LLVM_ENABLE_PROJECTS does not enable it.
set(COMPILER_RT_LIBCXX_PATH ${LLVM_EXTERNAL_LIBCXX_SOURCE_DIR}) set(COMPILER_RT_LIBCXX_PATH ${LLVM_EXTERNAL_LIBCXX_SOURCE_DIR})
▲ Show 20 Lines • Show All 67 Lines • Show Last 20 Lines

compiler-rt/lib/builtins/CMakeLists.txt

Show All 14 Lines list(INSERT CMAKE_MODULE_PATH 0
"${CMAKE_SOURCE_DIR}/../../../cmake" "${CMAKE_SOURCE_DIR}/../../../cmake"
"${CMAKE_SOURCE_DIR}/../../../cmake/Modules") "${CMAKE_SOURCE_DIR}/../../../cmake/Modules")
include(base-config-ix) include(base-config-ix)
include(CompilerRTUtils) include(CompilerRTUtils)
load_llvm_config() load_llvm_config()
construct_compiler_rt_default_triple() construct_compiler_rt_default_triple()
include(CheckPlatform)
if(APPLE) if(APPLE)
include(CompilerRTDarwinUtils) include(CompilerRTDarwinUtils)
endif() endif()
if(APPLE) if(APPLE)
include(UseLibtool) include(UseLibtool)
endif() endif()
include(AddCompilerRT) include(AddCompilerRT)
if(${CMAKE_SYSTEM_NAME} MATCHES "AIX")
set(CMAKE_C_ARCHIVE_CREATE "<CMAKE_AR> -X32_64 qc <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_CXX_ARCHIVE_CREATE "<CMAKE_AR> -X32_64 qc <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_C_ARCHIVE_APPEND "<CMAKE_AR> -X32_64 q <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_CXX_ARCHIVE_APPEND "<CMAKE_AR> -X32_64 q <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_C_ARCHIVE_FINISH "<CMAKE_RANLIB> -X32_64 <TARGET>")
set(CMAKE_CXX_ARCHIVE_FINISH "<CMAKE_RANLIB> -X32_64 <TARGET>")
endif()
endif() endif()
if (COMPILER_RT_STANDALONE_BUILD) if (COMPILER_RT_STANDALONE_BUILD)
# When compiler-rt is being built standalone, possibly as a cross-compilation # When compiler-rt is being built standalone, possibly as a cross-compilation
# target, the target may or may not want position independent code. This # target, the target may or may not want position independent code. This
# option provides an avenue through which the flag may be controlled when an # option provides an avenue through which the flag may be controlled when an
# LLVM configuration is not being utilized. # LLVM configuration is not being utilized.
option(COMPILER_RT_BUILTINS_ENABLE_PIC option(COMPILER_RT_BUILTINS_ENABLE_PIC
▲ Show 20 Lines • Show All 763 Lines • Show Last 20 Lines