subject:"\[PATCH\] D120727\: \[libc\+\+\] Overhaul how we select the ABI library"

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-26 Thread Mehdi AMINI via Phabricator via cfe-commits

mehdi_amini added a comment.

Forgot to mention, we configure this build with:

  cmake -G Ninja ${src_dir}/llvm -DLLVM_TARGETS_TO_BUILD="X86;NVPTX;AMDGPU" 
-DLLVM_ENABLE_PROJECTS="lld;clang;mlir" -DCMAKE_BUILD_TYPE=Release 
-DLLVM_ENABLE_ASSERTIONS=Off -DLIBCXX_CXX_ABI=default 
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi"  -DLLVM_ENABLE_LLD=ON  
-DLLVM_DISTRIBUTION_COMPONENTS="clang;runtimes" -DLLVM_CCACHE_BUILD=ON

I guess `-DLIBCXX_CXX_ABI=default` is the issue, I'm not sure what we should 
use here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-26 Thread Mehdi AMINI via Phabricator via cfe-commits

mehdi_amini added a comment.

I hit a config failure after f8c8bda965dd0f622de1ad3863b728661af6eb72 


  CMake Error at 
/var/lib/buildkite-agent/builds/buildkite-588bd64db9-mk2vq-1/mlir/mlir-core/libcxx/CMakeLists.txt:225
 (message):
Unsupported C++ ABI library: 'default'.  Supported values are
none;libcxxabi;system-libcxxabi;libcxxrt;libstdc++;libsupc++;vcruntime.
   

Here: 
https://buildkite.com/mlir/mlir-core/builds/22800#018100ac-56d6-4051-95ea-7dc91b98e658/14

We build from a clean directory, there is no CMake cache involved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-13 Thread Petr Hosek via Phabricator via cfe-commits

phosek added a comment.

D125597  should address the build error in 
compiler-rt.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-13 Thread Peter Collingbourne via Phabricator via cfe-commits

pcc added a comment.

In D120727#3512177 , @paulkirth wrote:

> Hi, we're seeing some failures in Fuchsia's Clang CI. Our runtimes build 
> seems to be unable to find `cxxabi.h`.
>
> The failing bot can be found here: 
> https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814278370664903633/overview

Looks like we're seeing the same issue on the sanitizer CI: 
https://lab.llvm.org/buildbot/#/builders/37/builds/13232


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-13 Thread Paul Kirth via Phabricator via cfe-commits

paulkirth added a comment.

Hi, we're seeing some failures in Fuchsia's Clang CI. Our runtimes build seems 
to be unable to find `cxxabi.h`.

The failing bot can be found here: 
https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8814278370664903633/overview


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-13 Thread Louis Dionne via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa80e65e00ada: [libc++] Overhaul how we select the ABI 
library (authored by ldionne).

Changed prior to commit:
  https://reviews.llvm.org/D120727?vs=429069=429209#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py
  libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
  libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
  libcxxabi/test/configs/cmake-bridge.cfg.in
  libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in

Index: libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
@@ -4,7 +4,7 @@
 
 config.substitutions.append(('%{flags}',''))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} ' +
 '-D__LIBC_NO_CPP_MATH_OVERLOADS__ -DLIBCXXABI_NO_TIMER ' +
 '-D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
Index: libcxxabi/test/configs/cmake-bridge.cfg.in
===
--- libcxxabi/test/configs/cmake-bridge.cfg.in
+++ libcxxabi/test/configs/cmake-bridge.cfg.in
@@ -28,7 +28,8 @@
 
 config.substitutions.append(('%{cxx}', '@CMAKE_CXX_COMPILER@'))
 config.substitutions.append(('%{libcxx}', '@LIBCXXABI_LIBCXX_PATH@'))
-config.substitutions.append(('%{include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
-config.substitutions.append(('%{target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
+config.substitutions.append(('%{include}', '@LIBCXXABI_SOURCE_DIR@/include'))
+config.substitutions.append(('%{cxx-include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
+config.substitutions.append(('%{cxx-target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
 config.substitutions.append(('%{lib}', '@LIBCXXABI_LIBRARY_DIR@'))
 config.substitutions.append(('%{executor}', '@LIBCXXABI_EXECUTOR@'))
Index: libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
@@ -6,7 +6,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
@@ -45,7 +45,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -412,7 +412,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-elif cxx_abi == 'none' or cxx_abi == 'default':
+elif cxx_abi == 'none':
 if self.target_info.is_windows():
 debug_suffix = 'd' if self.debug_build else ''
 self.cxx.link_flags += ['-lmsvcrt%s' % debug_suffix]
Index: libcxx/test/configs/legacy.cfg.in
===
--- libcxx/test/configs/legacy.cfg.in
+++ libcxx/test/configs/legacy.cfg.in
@@ -14,7

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-13 Thread Louis Dionne via Phabricator via cfe-commits

ldionne accepted this revision as: libc++.
ldionne marked an inline comment as done.
ldionne added inline comments.
This revision is now accepted and ready to land.



Comment at: libcxx/CMakeLists.txt:250
+
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt 
libstdc++ libsupc++ vcruntime)
+set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ 
ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")

phosek wrote:
> mstorsjo wrote:
> > ldionne wrote:
> > > phosek wrote:
> > > > All of these values refer to system ABI libraries except for libcxxabi 
> > > > where `libcxxabi is the in-tree one and `system-libcxxabi` is the 
> > > > system one. From a consistency perspective, I think it'd be better for 
> > > > `libcxxabi` to also refer to a system ABI library, and then use a 
> > > > different name for the in-tree one, perhaps `default`?
> > > I agree with the consistency argument. However, I'd like to avoid 
> > > `default` since it doesn't convey any information, and it also used to be 
> > > one of the valid values but it represented "whatever automatically 
> > > selected ABI we pick", which might not be libc++abi. I'd suggest 
> > > `libcxxabi` and `intree-libcxxabi`. It's not extremely pretty, but it 
> > > conveys exactly what it is.
> > > 
> > > Edit: I actually went ahead and made this change, and I realized that it 
> > > might be breaking for a bunch of people. Indeed, I think that most people 
> > > who specify `libcxxabi` are expecting to build against the in-tree one, 
> > > which is what happens today as long as they have `libcxxabi` in their 
> > > `LLVM_ENABLE_RUNTIMES`. With this renaming, they would implicitly start 
> > > building against the system libc++abi, and that might happen silently. 
> > > I'm not sure I like this. So, here's the diff to implement this change, 
> > > however I'd rather not apply it unless you think consistency is more 
> > > important that this concern:
> > > 
> > > ```
> > > diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
> > > index a7f1684235e6..7cbf8957ac90 100644
> > > --- a/libcxx/CMakeLists.txt
> > > +++ b/libcxx/CMakeLists.txt
> > > @@ -244,10 +244,10 @@ if (LIBCXX_TARGETING_MSVC)
> > >  elseif (${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD")
> > >set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxrt")
> > >  else()
> > > -  set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxabi")
> > > +  set(LIBCXX_DEFAULT_ABI_LIBRARY "intree-libcxxabi")
> > >  endif()
> > >  
> > > -set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi 
> > > libcxxrt libstdc++ libsupc++ vcruntime)
> > > +set(LIBCXX_SUPPORTED_ABI_LIBRARIES none intree-libcxxabi libcxxabi 
> > > libcxxrt libstdc++ libsupc++ vcruntime)
> > >  set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify 
> > > C++ ABI library to use. Supported values are 
> > > ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")
> > >  
> > >  # Temporary to still accept existing CMake caches that contain "default" 
> > > as the value
> > > diff --git a/libcxx/cmake/Modules/HandleLibCXXABI.cmake 
> > > b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> > > index 6e7a53258c0a..f22cfd674623 100644
> > > --- a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> > > +++ b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> > > @@ -97,7 +97,7 @@ add_library(libcxx-abi-headers INTERFACE)
> > >import_shared_library(libcxx-abi-static 
> > > "${LIBCXX_CXX_ABI_LIBRARY_PATH}" supc++)
> > >  
> > >  # Link against the in-tree libc++abi
> > > -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> > > +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "intree-libcxxabi")
> > >add_library(libcxx-abi-headers INTERFACE)
> > >target_link_libraries(libcxx-abi-headers INTERFACE cxxabi-headers)
> > >target_compile_definitions(libcxx-abi-headers INTERFACE 
> > > "-DLIBCXX_BUILDING_LIBCXXABI")
> > > @@ -111,7 +111,7 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> > >endif()
> > >  
> > >  # Link against a system-provided libc++abi
> > > -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
> > > +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> > >add_library(libcxx-abi-headers INTERFACE)
> > >import_private_headers(libcxx-abi-headers 
> > > "${LIBCXX_CXX_ABI_INCLUDE_PATHS}" "cxxabi.h;__cxxabi_config.h")
> > >target_compile_definitions(libcxx-abi-headers INTERFACE 
> > > "-DLIBCXX_BUILDING_LIBCXXABI")
> > > diff --git a/libcxx/cmake/caches/AIX.cmake b/libcxx/cmake/caches/AIX.cmake
> > > index 029b8deae3d7..e3f31aea5192 100644
> > > --- a/libcxx/cmake/caches/AIX.cmake
> > > +++ b/libcxx/cmake/caches/AIX.cmake
> > > @@ -13,4 +13,4 @@ set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
> > >  set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
> > >  set(LIBCXXABI_ENABLE_SHARED ON CACHE BOOL "")
> > >  set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
> > > -set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
> > > +set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-12 Thread Petr Hosek via Phabricator via cfe-commits

phosek accepted this revision.
phosek added a comment.

LGTM




Comment at: libcxx/CMakeLists.txt:250
+
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt 
libstdc++ libsupc++ vcruntime)
+set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ 
ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")

mstorsjo wrote:
> ldionne wrote:
> > phosek wrote:
> > > All of these values refer to system ABI libraries except for libcxxabi 
> > > where `libcxxabi is the in-tree one and `system-libcxxabi` is the system 
> > > one. From a consistency perspective, I think it'd be better for 
> > > `libcxxabi` to also refer to a system ABI library, and then use a 
> > > different name for the in-tree one, perhaps `default`?
> > I agree with the consistency argument. However, I'd like to avoid `default` 
> > since it doesn't convey any information, and it also used to be one of the 
> > valid values but it represented "whatever automatically selected ABI we 
> > pick", which might not be libc++abi. I'd suggest `libcxxabi` and 
> > `intree-libcxxabi`. It's not extremely pretty, but it conveys exactly what 
> > it is.
> > 
> > Edit: I actually went ahead and made this change, and I realized that it 
> > might be breaking for a bunch of people. Indeed, I think that most people 
> > who specify `libcxxabi` are expecting to build against the in-tree one, 
> > which is what happens today as long as they have `libcxxabi` in their 
> > `LLVM_ENABLE_RUNTIMES`. With this renaming, they would implicitly start 
> > building against the system libc++abi, and that might happen silently. I'm 
> > not sure I like this. So, here's the diff to implement this change, however 
> > I'd rather not apply it unless you think consistency is more important that 
> > this concern:
> > 
> > ```
> > diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
> > index a7f1684235e6..7cbf8957ac90 100644
> > --- a/libcxx/CMakeLists.txt
> > +++ b/libcxx/CMakeLists.txt
> > @@ -244,10 +244,10 @@ if (LIBCXX_TARGETING_MSVC)
> >  elseif (${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD")
> >set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxrt")
> >  else()
> > -  set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxabi")
> > +  set(LIBCXX_DEFAULT_ABI_LIBRARY "intree-libcxxabi")
> >  endif()
> >  
> > -set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi 
> > libcxxrt libstdc++ libsupc++ vcruntime)
> > +set(LIBCXX_SUPPORTED_ABI_LIBRARIES none intree-libcxxabi libcxxabi 
> > libcxxrt libstdc++ libsupc++ vcruntime)
> >  set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify 
> > C++ ABI library to use. Supported values are 
> > ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")
> >  
> >  # Temporary to still accept existing CMake caches that contain "default" 
> > as the value
> > diff --git a/libcxx/cmake/Modules/HandleLibCXXABI.cmake 
> > b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> > index 6e7a53258c0a..f22cfd674623 100644
> > --- a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> > +++ b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> > @@ -97,7 +97,7 @@ add_library(libcxx-abi-headers INTERFACE)
> >import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}" 
> > supc++)
> >  
> >  # Link against the in-tree libc++abi
> > -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> > +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "intree-libcxxabi")
> >add_library(libcxx-abi-headers INTERFACE)
> >target_link_libraries(libcxx-abi-headers INTERFACE cxxabi-headers)
> >target_compile_definitions(libcxx-abi-headers INTERFACE 
> > "-DLIBCXX_BUILDING_LIBCXXABI")
> > @@ -111,7 +111,7 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> >endif()
> >  
> >  # Link against a system-provided libc++abi
> > -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
> > +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> >add_library(libcxx-abi-headers INTERFACE)
> >import_private_headers(libcxx-abi-headers 
> > "${LIBCXX_CXX_ABI_INCLUDE_PATHS}" "cxxabi.h;__cxxabi_config.h")
> >target_compile_definitions(libcxx-abi-headers INTERFACE 
> > "-DLIBCXX_BUILDING_LIBCXXABI")
> > diff --git a/libcxx/cmake/caches/AIX.cmake b/libcxx/cmake/caches/AIX.cmake
> > index 029b8deae3d7..e3f31aea5192 100644
> > --- a/libcxx/cmake/caches/AIX.cmake
> > +++ b/libcxx/cmake/caches/AIX.cmake
> > @@ -13,4 +13,4 @@ set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
> >  set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
> >  set(LIBCXXABI_ENABLE_SHARED ON CACHE BOOL "")
> >  set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
> > -set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
> > +set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING "")
> > diff --git a/libcxx/cmake/caches/Apple.cmake 
> > b/libcxx/cmake/caches/Apple.cmake
> > index 4581d4d87b80..89087662b2f4 100644
> > --- a/libcxx/cmake/caches/Apple.cmake
> > +++ b/libcxx/cmake/caches/Apple.cmake
> > @@ -7,7 +7,7 @@ set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
> >

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-12 Thread Louis Dionne via Phabricator via cfe-commits

ldionne updated this revision to Diff 429069.
ldionne marked 2 inline comments as done.
ldionne added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py
  libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
  libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
  libcxxabi/test/configs/cmake-bridge.cfg.in
  libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in

Index: libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
@@ -4,7 +4,7 @@
 
 config.substitutions.append(('%{flags}',''))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} ' +
 '-D__LIBC_NO_CPP_MATH_OVERLOADS__ -DLIBCXXABI_NO_TIMER ' +
 '-D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
Index: libcxxabi/test/configs/cmake-bridge.cfg.in
===
--- libcxxabi/test/configs/cmake-bridge.cfg.in
+++ libcxxabi/test/configs/cmake-bridge.cfg.in
@@ -28,7 +28,8 @@
 
 config.substitutions.append(('%{cxx}', '@CMAKE_CXX_COMPILER@'))
 config.substitutions.append(('%{libcxx}', '@LIBCXXABI_LIBCXX_PATH@'))
-config.substitutions.append(('%{include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
-config.substitutions.append(('%{target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
+config.substitutions.append(('%{include}', '@LIBCXXABI_SOURCE_DIR@/include'))
+config.substitutions.append(('%{cxx-include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
+config.substitutions.append(('%{cxx-target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
 config.substitutions.append(('%{lib}', '@LIBCXXABI_LIBRARY_DIR@'))
 config.substitutions.append(('%{executor}', '@LIBCXXABI_EXECUTOR@'))
Index: libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
@@ -6,7 +6,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
@@ -45,7 +45,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -412,7 +412,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-elif cxx_abi == 'none' or cxx_abi == 'default':
+elif cxx_abi == 'none':
 if self.target_info.is_windows():
 debug_suffix = 'd' if self.debug_build else ''
 self.cxx.link_flags += ['-lmsvcrt%s' % debug_suffix]
Index: libcxx/test/configs/legacy.cfg.in
===
--- libcxx/test/configs/legacy.cfg.in
+++ libcxx/test/configs/legacy.cfg.in
@@ -14,7 +14,7 @@
 config.cxx_library_root = "@LIBCXX_LIBRARY_DIR@"
 config.abi_library_root =

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-12 Thread Martin Storsjö via Phabricator via cfe-commits

mstorsjo added inline comments.



Comment at: libcxx/CMakeLists.txt:250
+
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt 
libstdc++ libsupc++ vcruntime)
+set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ 
ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")

ldionne wrote:
> phosek wrote:
> > All of these values refer to system ABI libraries except for libcxxabi 
> > where `libcxxabi is the in-tree one and `system-libcxxabi` is the system 
> > one. From a consistency perspective, I think it'd be better for `libcxxabi` 
> > to also refer to a system ABI library, and then use a different name for 
> > the in-tree one, perhaps `default`?
> I agree with the consistency argument. However, I'd like to avoid `default` 
> since it doesn't convey any information, and it also used to be one of the 
> valid values but it represented "whatever automatically selected ABI we 
> pick", which might not be libc++abi. I'd suggest `libcxxabi` and 
> `intree-libcxxabi`. It's not extremely pretty, but it conveys exactly what it 
> is.
> 
> Edit: I actually went ahead and made this change, and I realized that it 
> might be breaking for a bunch of people. Indeed, I think that most people who 
> specify `libcxxabi` are expecting to build against the in-tree one, which is 
> what happens today as long as they have `libcxxabi` in their 
> `LLVM_ENABLE_RUNTIMES`. With this renaming, they would implicitly start 
> building against the system libc++abi, and that might happen silently. I'm 
> not sure I like this. So, here's the diff to implement this change, however 
> I'd rather not apply it unless you think consistency is more important that 
> this concern:
> 
> ```
> diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
> index a7f1684235e6..7cbf8957ac90 100644
> --- a/libcxx/CMakeLists.txt
> +++ b/libcxx/CMakeLists.txt
> @@ -244,10 +244,10 @@ if (LIBCXX_TARGETING_MSVC)
>  elseif (${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD")
>set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxrt")
>  else()
> -  set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxabi")
> +  set(LIBCXX_DEFAULT_ABI_LIBRARY "intree-libcxxabi")
>  endif()
>  
> -set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt 
> libstdc++ libsupc++ vcruntime)
> +set(LIBCXX_SUPPORTED_ABI_LIBRARIES none intree-libcxxabi libcxxabi libcxxrt 
> libstdc++ libsupc++ vcruntime)
>  set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ 
> ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")
>  
>  # Temporary to still accept existing CMake caches that contain "default" as 
> the value
> diff --git a/libcxx/cmake/Modules/HandleLibCXXABI.cmake 
> b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> index 6e7a53258c0a..f22cfd674623 100644
> --- a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> +++ b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
> @@ -97,7 +97,7 @@ add_library(libcxx-abi-headers INTERFACE)
>import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}" 
> supc++)
>  
>  # Link against the in-tree libc++abi
> -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
> +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "intree-libcxxabi")
>add_library(libcxx-abi-headers INTERFACE)
>target_link_libraries(libcxx-abi-headers INTERFACE cxxabi-headers)
>target_compile_definitions(libcxx-abi-headers INTERFACE 
> "-DLIBCXX_BUILDING_LIBCXXABI")
> @@ -111,7 +111,7 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
>endif()
>  
>  # Link against a system-provided libc++abi
> -elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
> +elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
>add_library(libcxx-abi-headers INTERFACE)
>import_private_headers(libcxx-abi-headers 
> "${LIBCXX_CXX_ABI_INCLUDE_PATHS}" "cxxabi.h;__cxxabi_config.h")
>target_compile_definitions(libcxx-abi-headers INTERFACE 
> "-DLIBCXX_BUILDING_LIBCXXABI")
> diff --git a/libcxx/cmake/caches/AIX.cmake b/libcxx/cmake/caches/AIX.cmake
> index 029b8deae3d7..e3f31aea5192 100644
> --- a/libcxx/cmake/caches/AIX.cmake
> +++ b/libcxx/cmake/caches/AIX.cmake
> @@ -13,4 +13,4 @@ set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
>  set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
>  set(LIBCXXABI_ENABLE_SHARED ON CACHE BOOL "")
>  set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
> -set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
> +set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING "")
> diff --git a/libcxx/cmake/caches/Apple.cmake b/libcxx/cmake/caches/Apple.cmake
> index 4581d4d87b80..89087662b2f4 100644
> --- a/libcxx/cmake/caches/Apple.cmake
> +++ b/libcxx/cmake/caches/Apple.cmake
> @@ -7,7 +7,7 @@ set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
>  set(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY OFF CACHE BOOL "")
>  set(LIBCXX_ENABLE_STATIC ON CACHE BOOL "")
>  set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
> -set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
> +set(LIBCXX_CXX_ABI

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-12 Thread Louis Dionne via Phabricator via cfe-commits

ldionne marked 5 inline comments as done.
ldionne added inline comments.



Comment at: libcxx/CMakeLists.txt:250
+
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt 
libstdc++ libsupc++ vcruntime)
+set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ 
ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")

phosek wrote:
> All of these values refer to system ABI libraries except for libcxxabi where 
> `libcxxabi is the in-tree one and `system-libcxxabi` is the system one. From 
> a consistency perspective, I think it'd be better for `libcxxabi` to also 
> refer to a system ABI library, and then use a different name for the in-tree 
> one, perhaps `default`?
I agree with the consistency argument. However, I'd like to avoid `default` 
since it doesn't convey any information, and it also used to be one of the 
valid values but it represented "whatever automatically selected ABI we pick", 
which might not be libc++abi. I'd suggest `libcxxabi` and `intree-libcxxabi`. 
It's not extremely pretty, but it conveys exactly what it is.

Edit: I actually went ahead and made this change, and I realized that it might 
be breaking for a bunch of people. Indeed, I think that most people who specify 
`libcxxabi` are expecting to build against the in-tree one, which is what 
happens today as long as they have `libcxxabi` in their `LLVM_ENABLE_RUNTIMES`. 
With this renaming, they would implicitly start building against the system 
libc++abi, and that might happen silently. I'm not sure I like this. So, here's 
the diff to implement this change, however I'd rather not apply it unless you 
think consistency is more important that this concern:

```
diff --git a/libcxx/CMakeLists.txt b/libcxx/CMakeLists.txt
index a7f1684235e6..7cbf8957ac90 100644
--- a/libcxx/CMakeLists.txt
+++ b/libcxx/CMakeLists.txt
@@ -244,10 +244,10 @@ if (LIBCXX_TARGETING_MSVC)
 elseif (${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD")
   set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxrt")
 else()
-  set(LIBCXX_DEFAULT_ABI_LIBRARY "libcxxabi")
+  set(LIBCXX_DEFAULT_ABI_LIBRARY "intree-libcxxabi")
 endif()
 
-set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt 
libstdc++ libsupc++ vcruntime)
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none intree-libcxxabi libcxxabi libcxxrt 
libstdc++ libsupc++ vcruntime)
 set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ 
ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")
 
 # Temporary to still accept existing CMake caches that contain "default" as 
the value
diff --git a/libcxx/cmake/Modules/HandleLibCXXABI.cmake 
b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
index 6e7a53258c0a..f22cfd674623 100644
--- a/libcxx/cmake/Modules/HandleLibCXXABI.cmake
+++ b/libcxx/cmake/Modules/HandleLibCXXABI.cmake
@@ -97,7 +97,7 @@ add_library(libcxx-abi-headers INTERFACE)
   import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}" 
supc++)
 
 # Link against the in-tree libc++abi
-elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
+elseif ("${LIBCXX_CXX_ABI}" STREQUAL "intree-libcxxabi")
   add_library(libcxx-abi-headers INTERFACE)
   target_link_libraries(libcxx-abi-headers INTERFACE cxxabi-headers)
   target_compile_definitions(libcxx-abi-headers INTERFACE 
"-DLIBCXX_BUILDING_LIBCXXABI")
@@ -111,7 +111,7 @@ elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
   endif()
 
 # Link against a system-provided libc++abi
-elseif ("${LIBCXX_CXX_ABI}" STREQUAL "system-libcxxabi")
+elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libcxxabi")
   add_library(libcxx-abi-headers INTERFACE)
   import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}" 
"cxxabi.h;__cxxabi_config.h")
   target_compile_definitions(libcxx-abi-headers INTERFACE 
"-DLIBCXX_BUILDING_LIBCXXABI")
diff --git a/libcxx/cmake/caches/AIX.cmake b/libcxx/cmake/caches/AIX.cmake
index 029b8deae3d7..e3f31aea5192 100644
--- a/libcxx/cmake/caches/AIX.cmake
+++ b/libcxx/cmake/caches/AIX.cmake
@@ -13,4 +13,4 @@ set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
 set(LIBCXX_ENABLE_STATIC OFF CACHE BOOL "")
 set(LIBCXXABI_ENABLE_SHARED ON CACHE BOOL "")
 set(LIBCXXABI_ENABLE_STATIC OFF CACHE BOOL "")
-set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
+set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING "")
diff --git a/libcxx/cmake/caches/Apple.cmake b/libcxx/cmake/caches/Apple.cmake
index 4581d4d87b80..89087662b2f4 100644
--- a/libcxx/cmake/caches/Apple.cmake
+++ b/libcxx/cmake/caches/Apple.cmake
@@ -7,7 +7,7 @@ set(LIBCXX_ABI_VERSION "1" CACHE STRING "")
 set(LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY OFF CACHE BOOL "")
 set(LIBCXX_ENABLE_STATIC ON CACHE BOOL "")
 set(LIBCXX_ENABLE_SHARED ON CACHE BOOL "")
-set(LIBCXX_CXX_ABI libcxxabi CACHE STRING "")
+set(LIBCXX_CXX_ABI intree-libcxxabi CACHE STRING "")
 set(LIBCXX_HIDE_FROM_ABI_PER_TU_BY_DEFAULT ON CACHE BOOL "")
 set(LIBCXX_ENABLE_DEBUG_MODE_SUPPORT OFF CACHE BOOL "")

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-12 Thread Petr Hosek via Phabricator via cfe-commits

phosek added inline comments.



Comment at: libcxx/CMakeLists.txt:250
+
+set(LIBCXX_SUPPORTED_ABI_LIBRARIES none libcxxabi system-libcxxabi libcxxrt 
libstdc++ libsupc++ vcruntime)
+set(LIBCXX_CXX_ABI "${LIBCXX_DEFAULT_ABI_LIBRARY}" CACHE STRING "Specify C++ 
ABI library to use. Supported values are ${LIBCXX_SUPPORTED_ABI_LIBRARIES}.")

All of these values refer to system ABI libraries except for libcxxabi where 
`libcxxabi is the in-tree one and `system-libcxxabi` is the system one. From a 
consistency perspective, I think it'd be better for `libcxxabi` to also refer 
to a system ABI library, and then use a different name for the in-tree one, 
perhaps `default`?



Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:76-77
+
+  add_library(libcxx-abi-shared SHARED IMPORTED GLOBAL)
+  target_link_libraries(libcxx-abi-shared INTERFACE libcxx-abi-headers)
+  import_shared_library(libcxx-abi-shared "${LIBCXX_CXX_ABI_LIBRARY_PATH}" 
stdc++)

I'd consider moving these two lines into `import_shared_library` to further 
reduce duplication.



Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:80-81
+
+  add_library(libcxx-abi-static STATIC IMPORTED GLOBAL)
+  target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
+  import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}" 
stdc++)

The same here, I'd consider moving these two lines into `import_static_library`.



Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:82
+  target_link_libraries(libcxx-abi-static INTERFACE libcxx-abi-headers)
+  import_shared_library(libcxx-abi-static "${LIBCXX_CXX_ABI_LIBRARY_PATH}" 
stdc++)
+

I think this should be `static`?



Comment at: libcxx/cmake/Modules/HandleLibCXXABI.cmake:86
+elseif ("${LIBCXX_CXX_ABI}" STREQUAL "libsupc++")
+add_library(libcxx-abi-headers INTERFACE)
+  import_private_headers(libcxx-abi-headers "${LIBCXX_CXX_ABI_INCLUDE_PATHS}"

The indentation here is off.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-11 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added inline comments.



Comment at: libcxxabi/CMakeLists.txt:201
 endif()
+message(STATUS "Using libc++abi testing configuration: 
${LIBCXXABI_TEST_CONFIG}")
 set(LIBCXXABI_TEST_PARAMS "" CACHE STRING

mstorsjo wrote:
> Unrelated to the rest of the patch?
Indeed, let me do that separately as a NFC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-11 Thread Louis Dionne via Phabricator via cfe-commits

ldionne updated this revision to Diff 428658.
ldionne marked 2 inline comments as done.
ldionne added a comment.

Rebase and address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py
  libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
  libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
  libcxxabi/test/configs/cmake-bridge.cfg.in
  libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in

Index: libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
@@ -4,7 +4,7 @@
 
 config.substitutions.append(('%{flags}',''))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} ' +
 '-D__LIBC_NO_CPP_MATH_OVERLOADS__ -DLIBCXXABI_NO_TIMER ' +
 '-D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
Index: libcxxabi/test/configs/cmake-bridge.cfg.in
===
--- libcxxabi/test/configs/cmake-bridge.cfg.in
+++ libcxxabi/test/configs/cmake-bridge.cfg.in
@@ -28,7 +28,8 @@
 
 config.substitutions.append(('%{cxx}', '@CMAKE_CXX_COMPILER@'))
 config.substitutions.append(('%{libcxx}', '@LIBCXXABI_LIBCXX_PATH@'))
-config.substitutions.append(('%{include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
-config.substitutions.append(('%{target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
+config.substitutions.append(('%{include}', '@LIBCXXABI_SOURCE_DIR@/include'))
+config.substitutions.append(('%{cxx-include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
+config.substitutions.append(('%{cxx-target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
 config.substitutions.append(('%{lib}', '@LIBCXXABI_LIBRARY_DIR@'))
 config.substitutions.append(('%{executor}', '@LIBCXXABI_EXECUTOR@'))
Index: libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
@@ -6,7 +6,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
@@ -45,7 +45,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -412,7 +412,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-elif cxx_abi == 'none' or cxx_abi == 'default':
+elif cxx_abi == 'none':
 if self.target_info.is_windows():
 debug_suffix = 'd' if self.debug_build else ''
 self.cxx.link_flags += ['-lmsvcrt%s' % debug_suffix]
Index: libcxx/test/configs/legacy.cfg.in
===
--- libcxx/test/configs/legacy.cfg.in
+++ libcxx/test/configs/legacy.cfg.in
@@ -14,7 +14,7 @@
 config.cxx_library_root = "@LIBCXX_LIBRARY_DIR@"
 config.abi_library_root =

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-10 Thread Martin Storsjö via Phabricator via cfe-commits

mstorsjo added a comment.

I gave it a quick readthrough, but I don't think I can give a qualified review 
of the bulk of it... I noticed a couple minor details though.




Comment at: libcxx/CMakeLists.txt:260
   else()
-set(LIBCXX_CXX_ABI_LIBNAME "default")
+set(LIBCXX_CXX_ABI "libcxxabi")
   endif()

Couldn't these lines be replaced with just a `set(LIBCXX_CXX_ABI 
${LIBCXX_DEFAULT_ABI_LIBRARY})`?



Comment at: libcxxabi/CMakeLists.txt:201
 endif()
+message(STATUS "Using libc++abi testing configuration: 
${LIBCXXABI_TEST_CONFIG}")
 set(LIBCXXABI_TEST_PARAMS "" CACHE STRING

Unrelated to the rest of the patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-10 Thread Louis Dionne via Phabricator via cfe-commits

ldionne updated this revision to Diff 428361.
ldionne added a comment.

Rebase to poke CI. Any interest in reviewing this @phosek @mstorsjo?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py
  libcxxabi/CMakeLists.txt
  libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
  libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
  libcxxabi/test/configs/cmake-bridge.cfg.in
  libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in

Index: libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
@@ -4,7 +4,7 @@
 
 config.substitutions.append(('%{flags}',''))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} ' +
 '-D__LIBC_NO_CPP_MATH_OVERLOADS__ -DLIBCXXABI_NO_TIMER ' +
 '-D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
Index: libcxxabi/test/configs/cmake-bridge.cfg.in
===
--- libcxxabi/test/configs/cmake-bridge.cfg.in
+++ libcxxabi/test/configs/cmake-bridge.cfg.in
@@ -28,7 +28,8 @@
 
 config.substitutions.append(('%{cxx}', '@CMAKE_CXX_COMPILER@'))
 config.substitutions.append(('%{libcxx}', '@LIBCXXABI_LIBCXX_PATH@'))
-config.substitutions.append(('%{include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
-config.substitutions.append(('%{target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
+config.substitutions.append(('%{include}', '@LIBCXXABI_SOURCE_DIR@/include'))
+config.substitutions.append(('%{cxx-include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
+config.substitutions.append(('%{cxx-target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
 config.substitutions.append(('%{lib}', '@LIBCXXABI_LIBRARY_DIR@'))
 config.substitutions.append(('%{executor}', '@LIBCXXABI_EXECUTOR@'))
Index: libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
@@ -6,7 +6,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
@@ -45,7 +45,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxxabi/CMakeLists.txt
===
--- libcxxabi/CMakeLists.txt
+++ libcxxabi/CMakeLists.txt
@@ -198,6 +198,7 @@
 if (NOT IS_ABSOLUTE "${LIBCXXABI_TEST_CONFIG}")
   set(LIBCXXABI_TEST_CONFIG "${CMAKE_CURRENT_SOURCE_DIR}/test/configs/${LIBCXXABI_TEST_CONFIG}")
 endif()
+message(STATUS "Using libc++abi testing configuration: ${LIBCXXABI_TEST_CONFIG}")
 set(LIBCXXABI_TEST_PARAMS "" CACHE STRING
 "A list of parameters to run the Lit test suite with.")
 
Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -412,7 +412,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-elif cxx_abi == 'none' or

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-09 Thread Louis Dionne via Phabricator via cfe-commits

ldionne updated this revision to Diff 428108.
ldionne added a comment.
Herald added a project: libc++abi.
Herald added a reviewer: libc++abi.

Try addressing CI issues.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py
  libcxxabi/CMakeLists.txt
  libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
  libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
  libcxxabi/test/configs/cmake-bridge.cfg.in
  libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in

Index: libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/ibm-libc++abi-shared.cfg.in
@@ -4,7 +4,7 @@
 
 config.substitutions.append(('%{flags}',''))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} ' +
 '-D__LIBC_NO_CPP_MATH_OVERLOADS__ -DLIBCXXABI_NO_TIMER ' +
 '-D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
Index: libcxxabi/test/configs/cmake-bridge.cfg.in
===
--- libcxxabi/test/configs/cmake-bridge.cfg.in
+++ libcxxabi/test/configs/cmake-bridge.cfg.in
@@ -28,7 +28,8 @@
 
 config.substitutions.append(('%{cxx}', '@CMAKE_CXX_COMPILER@'))
 config.substitutions.append(('%{libcxx}', '@LIBCXXABI_LIBCXX_PATH@'))
-config.substitutions.append(('%{include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
-config.substitutions.append(('%{target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
+config.substitutions.append(('%{include}', '@LIBCXXABI_SOURCE_DIR@/include'))
+config.substitutions.append(('%{cxx-include}', '@LIBCXXABI_HEADER_DIR@/include/c++/v1'))
+config.substitutions.append(('%{cxx-target-include}', '@LIBCXXABI_HEADER_DIR@/%{triple}/include/c++/v1'))
 config.substitutions.append(('%{lib}', '@LIBCXXABI_LIBRARY_DIR@'))
 config.substitutions.append(('%{executor}', '@LIBCXXABI_EXECUTOR@'))
Index: libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-shared.cfg.in
@@ -6,7 +6,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
===
--- libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
+++ libcxxabi/test/configs/apple-libc++abi-backdeployment.cfg.in
@@ -45,7 +45,7 @@
 '-isysroot {}'.format('@CMAKE_OSX_SYSROOT@') if '@CMAKE_OSX_SYSROOT@' else ''
 ))
 config.substitutions.append(('%{compile_flags}',
-'-nostdinc++ -I %{include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
+'-nostdinc++ -I %{include} -I %{cxx-include} -I %{cxx-target-include} -DLIBCXXABI_NO_TIMER -D_LIBCPP_ENABLE_CXX17_REMOVED_UNEXPECTED_FUNCTIONS ' +
 '-I %{libcxx}/test/support -I %{libcxx}/src'
 ))
 config.substitutions.append(('%{link_flags}',
Index: libcxxabi/CMakeLists.txt
===
--- libcxxabi/CMakeLists.txt
+++ libcxxabi/CMakeLists.txt
@@ -198,6 +198,7 @@
 if (NOT IS_ABSOLUTE "${LIBCXXABI_TEST_CONFIG}")
   set(LIBCXXABI_TEST_CONFIG "${CMAKE_CURRENT_SOURCE_DIR}/test/configs/${LIBCXXABI_TEST_CONFIG}")
 endif()
+message(STATUS "Using libc++abi testing configuration: ${LIBCXXABI_TEST_CONFIG}")
 set(LIBCXXABI_TEST_PARAMS "" CACHE STRING
 "A list of parameters to run the Lit test suite with.")
 
Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -412,7 +412,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-05-06 Thread Louis Dionne via Phabricator via cfe-commits

ldionne updated this revision to Diff 427721.
ldionne added a comment.

Rebase onto main


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py

Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -412,7 +412,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-elif cxx_abi == 'none' or cxx_abi == 'default':
+elif cxx_abi == 'none':
 if self.target_info.is_windows():
 debug_suffix = 'd' if self.debug_build else ''
 self.cxx.link_flags += ['-lmsvcrt%s' % debug_suffix]
Index: libcxx/test/configs/legacy.cfg.in
===
--- libcxx/test/configs/legacy.cfg.in
+++ libcxx/test/configs/legacy.cfg.in
@@ -14,7 +14,7 @@
 config.cxx_library_root = "@LIBCXX_LIBRARY_DIR@"
 config.abi_library_root = "@LIBCXX_CXX_ABI_LIBRARY_PATH@"
 config.enable_shared= @LIBCXX_LINK_TESTS_WITH_SHARED_LIBCXX@
-config.cxx_abi  = "@LIBCXX_CXX_ABI_LIBNAME@"
+config.cxx_abi  = "@LIBCXX_CXXABI_FOR_TESTS@"
 config.configuration_variant= "@LIBCXX_LIT_VARIANT@"
 config.host_triple  = "@LLVM_HOST_TRIPLE@"
 config.sysroot  = "@CMAKE_SYSROOT@"
Index: libcxx/test/CMakeLists.txt
===
--- libcxx/test/CMakeLists.txt
+++ libcxx/test/CMakeLists.txt
@@ -17,7 +17,9 @@
 # The tests shouldn't link to any ABI library when it has been linked into
 # libc++ statically or via a linker script.
 if (LIBCXX_ENABLE_STATIC_ABI_LIBRARY OR LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
-  set(LIBCXX_CXX_ABI_LIBNAME "none")
+  set(LIBCXX_CXXABI_FOR_TESTS "none")
+else()
+  set(LIBCXX_CXXABI_FOR_TESTS "${LIBCXX_CXX_ABI}")
 endif()
 
 # The tests shouldn't link to libunwind if we have a linker script which
Index: libcxx/src/CMakeLists.txt
===
--- libcxx/src/CMakeLists.txt
+++ libcxx/src/CMakeLists.txt
@@ -156,11 +156,6 @@
   set(exclude_from_all EXCLUDE_FROM_ALL)
 endif()
 
-# If LIBCXX_CXX_ABI_LIBRARY_PATH is defined we want to add it to the search path.
-add_link_flags_if(LIBCXX_CXX_ABI_LIBRARY_PATH
-  "${CMAKE_LIBRARY_PATH_FLAG}${LIBCXX_CXX_ABI_LIBRARY_PATH}")
-
-
 if (LIBCXX_GENERATE_COVERAGE AND NOT LIBCXX_COVERAGE_LIBRARY)
   find_compiler_rt_library(profile LIBCXX_COVERAGE_LIBRARY)
 endif()
@@ -197,10 +192,6 @@
 endif()
 
 function(cxx_set_common_defines name)
-  if(LIBCXX_CXX_ABI_HEADER_TARGET)
-add_dependencies(${name} ${LIBCXX_CXX_ABI_HEADER_TARGET})
-  endif()
-
   if (LIBCXX_ENABLE_PARALLEL_ALGORITHMS)
 target_link_libraries(${name} PUBLIC pstl::ParallelSTL)
   endif()
@@ -241,19 +232,18 @@
   # Link against libc++abi
   if (LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
 if (APPLE)
-  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
+  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" "$")
 else()
-  target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}" "-Wl,-Bdynamic,--no-whole-archive")
+  target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic" "$" "-Wl,-Bdynamic,--no-whole-archive")
 endif()
   else()
-target_link_libraries(cxx_shared PUBLIC "${LIBCXX_CXX_SHARED_ABI_LIBRARY}")
+target_link_libraries(cxx_shared PUBLIC libcxx-abi-shared)
   endif()
 
   # Maybe re-export symbols from libc++abi
   # In particular, we don't re-export the symbols if libc++abi is merged statically
   # into libc++ because in that case there's no dylib to re-export from.
-  if (APPLE AND (LIBCXX_CXX_ABI_LIBNAME STREQUAL "libcxxabi" OR
- LIBCXX_CXX_ABI_LIBNAME STREQUAL "default")
+  if (APPLE AND LIBCXX_CXX_ABI STREQUAL "libcxxabi"
 AND NOT DEFINED LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS
 AND NOT LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
 set(LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS ON)
@@ -292,7 +282,8 @@
   add_library(cxx_static STATIC ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
   target_include_directories(cxx_static PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
   target_link_libraries(cxx_static PUBLIC cxx-headers
-

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-03-10 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added a comment.

In D120727#3372594 , @mstorsjo wrote:

> FWIW I think D116689  interacts with this 
> somewhat. I think D116689  is useful for 
> the fixes I want to do wrt libcxxabi integration on Windows (but I haven't 
> studied it, nor this one, closely yet though). @phosek Do you see anything in 
> this one that isn't compatible with D116689 
> ?

It does interact a lot, in fact. If we land this patch, I suspect that the 
libc++abi parts of D116689  may not be 
necessary anymore. And I also had a plan (not concrete) to use a similar 
approach for deciding how the unwind library is picked up, which may be worth 
exploring. As far as I can tell, the main thing that is nicer with D116689 
's approach over this patch is that we can 
get rid of logic around `"-Wl,-force_load"` for merging the static library into 
libc++, but I think the approach proposed here would be compatible with that 
too.

The thing I like about this approach is that it simplifies the amount of logic 
we need by properly encoding dependencies in the `libcxx-abi-shared|static` 
targets.

Comment at: libcxx/src/CMakeLists.txt:233-239
 if (APPLE)
-  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" 
"${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
+  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" 
"$")
 else()
-  target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic" 
"${LIBCXX_CXX_STATIC_ABI_LIBRARY}" "-Wl,-Bdynamic,--no-whole-archive")
+  target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic" 
"$" "-Wl,-Bdynamic,--no-whole-archive")
 endif()
   else()
+target_link_libraries(cxx_shared PUBLIC libcxx-abi-shared)

This part would become nicer if we had an `OBJECT` library like in D116689. I 
think this approach does not preclude improving this in the future by using an 
object library.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-03-10 Thread Martin Storsjö via Phabricator via cfe-commits

mstorsjo added subscribers: phosek, mstorsjo.
mstorsjo added a comment.

FWIW I think D116689  interacts with this 
somewhat. I think D116689  is useful for the 
fixes I want to do wrt libcxxabi integration on Windows (but I haven't studied 
it, nor this one, closely yet though). @phosek Do you see anything in this one 
that isn't compatible with D116689 ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-03-01 Thread Louis Dionne via Phabricator via cfe-commits

ldionne updated this revision to Diff 412191.
ldionne added a comment.

Rebase onto main.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120727/new/

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py

Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -419,7 +419,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-elif cxx_abi == 'none' or cxx_abi == 'default':
+elif cxx_abi == 'none':
 if self.target_info.is_windows():
 debug_suffix = 'd' if self.debug_build else ''
 self.cxx.link_flags += ['-lmsvcrt%s' % debug_suffix]
Index: libcxx/test/configs/legacy.cfg.in
===
--- libcxx/test/configs/legacy.cfg.in
+++ libcxx/test/configs/legacy.cfg.in
@@ -14,7 +14,7 @@
 config.cxx_library_root = "@LIBCXX_LIBRARY_DIR@"
 config.abi_library_root = "@LIBCXX_CXX_ABI_LIBRARY_PATH@"
 config.enable_shared= @LIBCXX_LINK_TESTS_WITH_SHARED_LIBCXX@
-config.cxx_abi  = "@LIBCXX_CXX_ABI_LIBNAME@"
+config.cxx_abi  = "@LIBCXX_CXXABI_FOR_TESTS@"
 config.configuration_variant= "@LIBCXX_LIT_VARIANT@"
 config.host_triple  = "@LLVM_HOST_TRIPLE@"
 config.sysroot  = "@CMAKE_SYSROOT@"
Index: libcxx/test/CMakeLists.txt
===
--- libcxx/test/CMakeLists.txt
+++ libcxx/test/CMakeLists.txt
@@ -17,7 +17,9 @@
 # The tests shouldn't link to any ABI library when it has been linked into
 # libc++ statically or via a linker script.
 if (LIBCXX_ENABLE_STATIC_ABI_LIBRARY OR LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
-  set(LIBCXX_CXX_ABI_LIBNAME "none")
+  set(LIBCXX_CXXABI_FOR_TESTS "none")
+else()
+  set(LIBCXX_CXXABI_FOR_TESTS "${LIBCXX_CXX_ABI}")
 endif()
 
 # The tests shouldn't link to libunwind if we have a linker script which
Index: libcxx/src/CMakeLists.txt
===
--- libcxx/src/CMakeLists.txt
+++ libcxx/src/CMakeLists.txt
@@ -155,11 +155,6 @@
   set(exclude_from_all EXCLUDE_FROM_ALL)
 endif()
 
-# If LIBCXX_CXX_ABI_LIBRARY_PATH is defined we want to add it to the search path.
-add_link_flags_if(LIBCXX_CXX_ABI_LIBRARY_PATH
-  "${CMAKE_LIBRARY_PATH_FLAG}${LIBCXX_CXX_ABI_LIBRARY_PATH}")
-
-
 if (LIBCXX_GENERATE_COVERAGE AND NOT LIBCXX_COVERAGE_LIBRARY)
   find_compiler_rt_library(profile LIBCXX_COVERAGE_LIBRARY)
 endif()
@@ -196,10 +191,6 @@
 endif()
 
 function(cxx_set_common_defines name)
-  if(LIBCXX_CXX_ABI_HEADER_TARGET)
-add_dependencies(${name} ${LIBCXX_CXX_ABI_HEADER_TARGET})
-  endif()
-
   if (LIBCXX_ENABLE_PARALLEL_ALGORITHMS)
 target_link_libraries(${name} PUBLIC pstl::ParallelSTL)
   endif()
@@ -240,19 +231,18 @@
   # Link against libc++abi
   if (LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
 if (APPLE)
-  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
+  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" "$")
 else()
-  target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}" "-Wl,-Bdynamic,--no-whole-archive")
+  target_link_libraries(cxx_shared PRIVATE "-Wl,--whole-archive,-Bstatic" "$" "-Wl,-Bdynamic,--no-whole-archive")
 endif()
   else()
-target_link_libraries(cxx_shared PUBLIC "${LIBCXX_CXX_SHARED_ABI_LIBRARY}")
+target_link_libraries(cxx_shared PUBLIC libcxx-abi-shared)
   endif()
 
   # Maybe re-export symbols from libc++abi
   # In particular, we don't re-export the symbols if libc++abi is merged statically
   # into libc++ because in that case there's no dylib to re-export from.
-  if (APPLE AND (LIBCXX_CXX_ABI_LIBNAME STREQUAL "libcxxabi" OR
- LIBCXX_CXX_ABI_LIBNAME STREQUAL "default")
+  if (APPLE AND LIBCXX_CXX_ABI STREQUAL "libcxxabi"
 AND NOT DEFINED LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS
 AND NOT LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
 set(LIBCXX_OSX_REEXPORT_LIBCXXABI_SYMBOLS ON)
@@ -291,7 +281,8 @@
   add_library(cxx_static STATIC ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
   target_include_directories(cxx_static PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
   target_link_libraries(cxx_static PUBLIC cxx-headers

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

2022-03-01 Thread Louis Dionne via Phabricator via cfe-commits

ldionne created this revision.
Herald added a subscriber: arichardson.
ldionne requested review of this revision.
Herald added projects: clang, libc++.
Herald added subscribers: libcxx-commits, cfe-commits.
Herald added a reviewer: libc++.

This patch overhauls how we pick up the ABI library. Instead of setting
ad-hoc flags, it creates interface targets that can be linked against by
the rest of the build, which is easier to follow and extend to support
new ABI libraries. It also adds a new ABI library called "system-libcxxabi",
which represents linking against a LLVM libc++abi already installed on the
system, and solves existing issues when trying to build against a system
libc++abi outside of the now-unsupported standalone build.

This is intended to be a NFC change, however there are some additional
simplifications and improvements we can make in the future that would
require a slight behavior change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120727

Files:
  clang/cmake/caches/CrossWinToARMLinux.cmake
  libcxx/CMakeLists.txt
  libcxx/cmake/Modules/HandleLibCXXABI.cmake
  libcxx/docs/BuildingLibcxx.rst
  libcxx/include/CMakeLists.txt
  libcxx/lib/abi/CMakeLists.txt
  libcxx/src/CMakeLists.txt
  libcxx/test/CMakeLists.txt
  libcxx/test/configs/legacy.cfg.in
  libcxx/utils/libcxx/test/config.py

Index: libcxx/utils/libcxx/test/config.py
===
--- libcxx/utils/libcxx/test/config.py
+++ libcxx/utils/libcxx/test/config.py
@@ -390,7 +390,7 @@
 self.cxx.link_flags += ['-lstdc++']
 elif cxx_abi == 'libsupc++':
 self.cxx.link_flags += ['-lsupc++']
-elif cxx_abi == 'libcxxabi':
+elif cxx_abi == 'libcxxabi' or 'system-libcxxabi':
 # If the C++ library requires explicitly linking to libc++abi, or
 # if we're testing libc++abi itself (the test configs are shared),
 # then link it.
@@ -417,7 +417,7 @@
 # The compiler normally links in oldnames.lib too, but we've
 # specified -nostdlib above, so we need to specify it manually.
 self.cxx.link_flags += ['-loldnames']
-elif cxx_abi == 'none' or cxx_abi == 'default':
+elif cxx_abi == 'none':
 if self.target_info.is_windows():
 debug_suffix = 'd' if self.debug_build else ''
 self.cxx.link_flags += ['-lmsvcrt%s' % debug_suffix]
Index: libcxx/test/configs/legacy.cfg.in
===
--- libcxx/test/configs/legacy.cfg.in
+++ libcxx/test/configs/legacy.cfg.in
@@ -14,7 +14,7 @@
 config.cxx_library_root = "@LIBCXX_LIBRARY_DIR@"
 config.abi_library_root = "@LIBCXX_CXX_ABI_LIBRARY_PATH@"
 config.enable_shared= @LIBCXX_LINK_TESTS_WITH_SHARED_LIBCXX@
-config.cxx_abi  = "@LIBCXX_CXX_ABI_LIBNAME@"
+config.cxx_abi  = "@LIBCXX_CXXABI_FOR_TESTS@"
 config.configuration_variant= "@LIBCXX_LIT_VARIANT@"
 config.host_triple  = "@LLVM_HOST_TRIPLE@"
 config.sysroot  = "@CMAKE_SYSROOT@"
Index: libcxx/test/CMakeLists.txt
===
--- libcxx/test/CMakeLists.txt
+++ libcxx/test/CMakeLists.txt
@@ -17,7 +17,9 @@
 # The tests shouldn't link to any ABI library when it has been linked into
 # libc++ statically or via a linker script.
 if (LIBCXX_ENABLE_STATIC_ABI_LIBRARY OR LIBCXX_ENABLE_ABI_LINKER_SCRIPT)
-  set(LIBCXX_CXX_ABI_LIBNAME "none")
+  set(LIBCXX_CXXABI_FOR_TESTS "none")
+else()
+  set(LIBCXX_CXXABI_FOR_TESTS "${LIBCXX_CXX_ABI}")
 endif()
 
 # The tests shouldn't link to libunwind if we have a linker script which
Index: libcxx/src/CMakeLists.txt
===
--- libcxx/src/CMakeLists.txt
+++ libcxx/src/CMakeLists.txt
@@ -155,11 +155,6 @@
   set(exclude_from_all EXCLUDE_FROM_ALL)
 endif()
 
-# If LIBCXX_CXX_ABI_LIBRARY_PATH is defined we want to add it to the search path.
-add_link_flags_if(LIBCXX_CXX_ABI_LIBRARY_PATH
-  "${CMAKE_LIBRARY_PATH_FLAG}${LIBCXX_CXX_ABI_LIBRARY_PATH}")
-
-
 if (LIBCXX_GENERATE_COVERAGE AND NOT LIBCXX_COVERAGE_LIBRARY)
   find_compiler_rt_library(profile LIBCXX_COVERAGE_LIBRARY)
 endif()
@@ -196,10 +191,6 @@
 endif()
 
 function(cxx_set_common_defines name)
-  if(LIBCXX_CXX_ABI_HEADER_TARGET)
-add_dependencies(${name} ${LIBCXX_CXX_ABI_HEADER_TARGET})
-  endif()
-
   if (LIBCXX_ENABLE_PARALLEL_ALGORITHMS)
 target_link_libraries(${name} PUBLIC pstl::ParallelSTL)
   endif()
@@ -240,19 +231,18 @@
   # Link against libc++abi
   if (LIBCXX_STATICALLY_LINK_ABI_IN_SHARED_LIBRARY)
 if (APPLE)
-  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" "${LIBCXX_CXX_STATIC_ABI_LIBRARY}")
+  target_link_libraries(cxx_shared PRIVATE "-Wl,-force_load" "$")
 else()
-  target_link_libraries(cxx_shared PRIVATE

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

[PATCH] D120727: [libc++] Overhaul how we select the ABI library

22 matches

Site Navigation

Mail list logo

Footer information