This is an archive of the discontinued LLVM Phabricator instance.

Differential D69169

[libcxx] Force-cache LIBCXX_CXX_ABI_LIBRARY_PATH
ClosedPublic

Authored by broadwaylamb on Oct 18 2019, 7:52 AM.

Details

Reviewers
jroelofs
bcraig
ldionne
EricWF
mclow.lists
vvereschaka
eastig
Commits
rG41f4dfd63ea0: [libcxx] Force-cache LIBCXX_CXX_ABI_LIBRARY_PATH
Summary

The LIBCXX_CXX_ABI_LIBRARY_PATH CMake variable is cached once in
libcxx/cmake/Modules/HandleLibCXXABI.cmake in the setup_abi_lib macro,
and then cached again in libcxx/test/CMakeLists.txt. There, if it is
not set to a value, it is by default set to LIBCXX_LIBRARY_DIR.

However, this new value is not actually cached, because the old (empty)
value has been already cached. Use the FORCE CMake flag so that it
is saved to the cache.

This should not break anything, because the code changed here previously
had no effect, when it should have.

Diff Detail

Event Timeline

broadwaylamb created this revision.Oct 18 2019, 7:52 AM
Herald added a reviewer: mclow.lists. · View Herald TranscriptOct 18 2019, 7:52 AM
Herald added subscribers: dexonsmith, christof, mgorny. · View Herald Transcript
Harbormaster completed remote builds in B39779: Diff 225621.Oct 18 2019, 8:01 AM
broadwaylamb added a comment.Nov 28 2019, 12:59 AM

Can anyone review this small modification?

broadwaylamb updated this revision to Diff 232608.Dec 6 2019, 10:34 AM

Remove unrelated change

broadwaylamb edited the summary of this revision. (Show Details)Dec 6 2019, 10:34 AM
Harbormaster completed remote builds in B42025: Diff 232608.Dec 6 2019, 10:41 AM
broadwaylamb added reviewers: vvereschaka, eastig.Dec 6 2019, 2:24 PM
broadwaylamb added a comment.Jan 9 2020, 7:32 AM

@jroelofs would you mind reviewing this one too, please?

vvereschaka accepted this revision.Jan 9 2020, 10:57 PM

LGTM

This revision is now accepted and ready to land.Jan 9 2020, 10:57 PM
Closed by commit rG41f4dfd63ea0: [libcxx] Force-cache LIBCXX_CXX_ABI_LIBRARY_PATH (authored by broadwaylamb). · Explain WhyJan 10 2020, 1:59 AM
This revision was automatically updated to reflect the committed changes.
mgorny added a comment.Jan 31 2020, 1:30 AM

This change broke stand-alone builds. They now try to use libc++abi from the wrong directory, instead of system instance:

clang-10: error: no such file or directory: '/home/mgorny/llvm-project/libcxx/build/lib64/libc++abi.a'
mgorny added a comment.Jan 31 2020, 1:31 AM

That said, can we revert this from 10.x branch, please?

broadwaylamb added a comment.Jan 31 2020, 3:09 AM
In D69169#1851257, @mgorny wrote:

That said, can we revert this from 10.x branch, please?

I'm investigating it right now, thanks for pointing this out.

broadwaylamb added a comment.Jan 31 2020, 3:31 AM

Could you please show me how you're building libc++? I'm having trouble reproducing the issue.

broadwaylamb added a comment.EditedJan 31 2020, 3:38 AM

Anyway, reverted from 10.x in 3573526c0286c9461f0459be1a4592b2214594e7.

mgorny added a comment.Jan 31 2020, 6:53 AM
In D69169#1851415, @broadwaylamb wrote:

Could you please show me how you're building libc++? I'm having trouble reproducing the issue.

cmake <path-to-libc++-src> -G Ninja -DLIBCXX_LIBDIR_SUFFIX=64 -DLIBCXX_ENABLE_SHARED=ON -DLIBCXX_ENABLE_STATIC=yes -DLIBCXX_CXX_ABI=libcxxabi -DLIBCXX_CXX_ABI_INCLUDE_PATHS=/usr/include/libcxxabi -DLIBCXX_ENABLE_ABI_LINKER_SCRIPT=OFF -DLIBCXX_HAS_MUSL_LIBC=no -DLIBCXX_HAS_GCC_S_LIB=ON -DLIBCXX_INCLUDE_TESTS=yes -DLIBCXX_USE_COMPILER_RT=OFF -DCMAKE_SHARED_LINKER_FLAGS="-lunwind -Wl,-O1 -Wl,--as-needed" -DLLVM_EXTERNAL_LIT=/usr/bin/lit -DLLVM_LIT_ARGS="-vv;-j;32;--param=cxx_under_test=/usr/lib/llvm/10/bin/x86_64-pc-linux-gnu-clang" -DCMAKE_BUILD_TYPE=RelWithDebInfo &&
ninja check-cxx
hans added a subscriber: hans.Feb 3 2020, 2:06 AM
In D69169#1851438, @broadwaylamb wrote:

Anyway, reverted from 10.x in 3573526c0286c9461f0459be1a4592b2214594e7.

Please don't push directly to the release branch without checking with me. Also, commits should go to master first, before being cherry-picked to any release branches.

I've cherry-picked the revert to master in 1a7e688b0b5a986fbbbce62ff1506a5991b0fce0

Please see http://lists.llvm.org/pipermail/llvm-dev/2020-January/138295.html for more info about the branch.

broadwaylamb added a comment.Feb 3 2020, 2:26 AM
In D69169#1854224, @hans wrote:
In D69169#1851438, @broadwaylamb wrote:

Anyway, reverted from 10.x in 3573526c0286c9461f0459be1a4592b2214594e7.

Please don't push directly to the release branch without checking with me. Also, commits should go to master first, before being cherry-picked to any release branches.

I've cherry-picked the revert to master in 1a7e688b0b5a986fbbbce62ff1506a5991b0fce0

Please see http://lists.llvm.org/pipermail/llvm-dev/2020-January/138295.html for more info about the branch.

Understood. Sorry.

broadwaylamb added a comment.Feb 10 2020, 8:00 AM

@mgorny I still can't reproduce the issue. What Linux distro are you using and how did you install your LLVM toolchain?

broadwaylamb mentioned this in D74347: [CMake] Explicitly specify paths to libc++abi in CrossWinToARMLinux.cmake.Feb 10 2020, 10:29 AM
mgorny added a comment.Feb 10 2020, 10:34 AM

Gentoo, using the ebuilds. I think the most relevant is how libc++abi is configured:

cmake -G Ninja -DCMAKE_INSTALL_PREFIX=/usr -DLIBCXXABI_LIBDIR_SUFFIX=64 -DLIBCXXABI_ENABLE_SHARED=ON -DLIBCXXABI_ENABLE_STATIC=yes -DLIBCXXABI_USE_LLVM_UNWINDER=yes -DLIBCXXABI_INCLUDE_TESTS=no -DLIBCXXABI_USE_COMPILER_RT=OFF -DLIBCXXABI_LIBCXX_INCLUDES=<path-to-tree>/libcxx/include -DLIBCXXABI_LIBUNWIND_INCLUDES=/usr/include -DCMAKE_BUILD_TYPE=RelWithDebInfo <path>

After normal ninja install, we additionally copy headers (contents of include directory) to /usr/include/libcxxabi, i.e.:

$ tree /usr/include/libcxxabi/
/usr/include/libcxxabi/
├── __cxxabi_config.h
└── cxxabi.h

0 directories, 2 files
broadwaylamb added a comment.Feb 11 2020, 1:27 AM

@mgorny Thank you. I've been able to reproduce it on my Ubuntu installation.

broadwaylamb mentioned this in rG801627ee490e: [CMake] Explicitly specify paths to libc++abi in CrossWinToARMLinux.cmake.Feb 17 2020, 4:27 AM
broadwaylamb mentioned this in rG03993f3b6f46: [CMake] Explicitly specify paths to libc++abi in CrossWinToARMLinux.cmake.Mar 13 2020, 8:01 AM

Revision Contents

PathSize
libcxx/
test/
2 lines

Diff 237269

libcxx/test/CMakeLists.txt

Show All 40 Lines
pythonize_bool(LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY) pythonize_bool(LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY)
pythonize_bool(LIBCXX_DEBUG_BUILD) pythonize_bool(LIBCXX_DEBUG_BUILD)
pythonize_bool(LIBCXX_ENABLE_PARALLEL_ALGORITHMS) pythonize_bool(LIBCXX_ENABLE_PARALLEL_ALGORITHMS)
# By default, for non-standalone builds, libcxx and libcxxabi share a library # By default, for non-standalone builds, libcxx and libcxxabi share a library
# directory. # directory.
if (NOT LIBCXX_CXX_ABI_LIBRARY_PATH) if (NOT LIBCXX_CXX_ABI_LIBRARY_PATH)
set(LIBCXX_CXX_ABI_LIBRARY_PATH "${LIBCXX_LIBRARY_DIR}" CACHE PATH set(LIBCXX_CXX_ABI_LIBRARY_PATH "${LIBCXX_LIBRARY_DIR}" CACHE PATH
"The path to libc++abi library.") "The path to libc++abi library." FORCE)
endif() endif()
set(LIBCXX_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING set(LIBCXX_TARGET_INFO "libcxx.test.target_info.LocalTI" CACHE STRING
"TargetInfo to use when setting up test environment.") "TargetInfo to use when setting up test environment.")
set(LIBCXX_EXECUTOR "None" CACHE STRING set(LIBCXX_EXECUTOR "None" CACHE STRING
"Executor to use when running tests.") "Executor to use when running tests.")
set(AUTO_GEN_COMMENT "## Autogenerated by libcxx configuration.\n# Do not edit!") set(AUTO_GEN_COMMENT "## Autogenerated by libcxx configuration.\n# Do not edit!")
▲ Show 20 Lines • Show All 75 Lines • Show Last 20 Lines