This is an archive of the discontinued LLVM Phabricator instance.

[runtimes] Properly handle the sysroot/triple/gcc-toolchain
ClosedPublic

Authored by ldionne on Oct 12 2021, 1:05 PM.

Details

Reviewers
phosek
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rG72117f2ffeb6: [runtimes] Properly handle the sysroot/triple/gcc-toolchain
Summary

In 395271a, I simplified how we handled the target triple for the
runtimes. However, in doing so, we stopped considering the default
in CMAKE_CXX_COMPILER_TARGET, so we'd use the LLVM_DEFAULT_TARGET_TRIPLE
(which is the host triple) even if CMAKE_CXX_COMPILER_TARGET was specified.
This commit fixes that problem and also refactors the code so that it's
easy to see what the default value is.

The fact that nobody seems to have been broken by this makes me think
that perhaps nobody is using CMAKE_CXX_COMPILER_TARGET to specify the
triple -- but it should still work.

Diff Detail

Event Timeline

ldionne created this revision.Oct 12 2021, 1:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2021, 1:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.Oct 12 2021, 1:05 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 12 2021, 1:05 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

Gentle ping @phosek , this is pretty bad actually, since we currently don't honour building for the target requested by the user. I'd like to fix this ASAP and even merge the fix to release/13.x.

I wonder if we can deprecate LIBCXX_TARGET_TRIPLE, LIBCXX_SYSROOT, and LIBCXX_GCC_TOOLCHAIN (and their libc++abi and libunwind counterparts), and switch to the CMake variables instead? Do those custom variables give us anything?

libcxx/CMakeLists.txt
281–283

This changes the semantics slightly. Previously the values of LIBCXX_TARGET_TRIPLE, LIBCXX_SYSROOT, and LIBCXX_GCC_TOOLCHAIN wouldn't be cached, so if you changed CMAKE_CXX_COMPILER_TARGET, CMAKE_SYSROOT, or CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN those changes would be immediately picked. Now, you'll have to modify LIBCXX_TARGET_TRIPLE, LIBCXX_SYSROOT, and LIBCXX_GCC_TOOLCHAIN instead because any subsequent changes to CMAKE_CXX_COMPILER_TARGET, CMAKE_SYSROOT, or CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN would be ignored.

I wonder if we can deprecate LIBCXX_TARGET_TRIPLE, LIBCXX_SYSROOT, and LIBCXX_GCC_TOOLCHAIN (and their libc++abi and libunwind counterparts), and switch to the CMake variables instead? Do those custom variables give us anything?

I thought you added those variables for the Bootstrapping build? I'm more than happy to remove them if you don't need them.

libcxx/CMakeLists.txt
281–283

Oh, that's right. It does seem to be more consistent with that we do for other per-project setting, though.

phosek accepted this revision.Oct 21 2021, 1:49 AM

LGTM

I wonder if we can deprecate LIBCXX_TARGET_TRIPLE, LIBCXX_SYSROOT, and LIBCXX_GCC_TOOLCHAIN (and their libc++abi and libunwind counterparts), and switch to the CMake variables instead? Do those custom variables give us anything?

I thought you added those variables for the Bootstrapping build? I'm more than happy to remove them if you don't need them.

I don't think they should be needed anymore, but it's possible that other parts of the build depend on them. In any case, this can be addressed in a follow up change.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 21 2021, 7:06 AM
This revision was automatically updated to reflect the committed changes.