[libcxx-commits] [PATCH] D128927: [libc++] Always build c++experimental.a

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

It looks like the tests would work in CI in the clang-cl configurations with this modification:

  diff --git a/libcxx/utils/ci/run-buildbot b/libcxx/utils/ci/run-buildbot
  index 65155ba173ed..407e93ef1575 100755
  --- a/libcxx/utils/ci/run-buildbot
  +++ b/libcxx/utils/ci/run-buildbot
  @@ -114,8 +114,6 @@ function generate-cmake-libcxx-win() {
             -DCMAKE_CXX_COMPILER=clang-cl \
             -DLIBCXX_ENABLE_FILESYSTEM=YES \
             -DLIBCXX_EXTRA_SITE_DEFINES="_LIBCPP_HAS_NO_INT128" \
  -          -DLIBCXX_TEST_PARAMS="enable_experimental=False" \
  -          -DLIBCXXABI_TEST_PARAMS="enable_experimental=False" \
             "${@}"
   }
   
  @@ -521,7 +519,7 @@ armv7-noexceptions)
   ;;
   clang-cl-dll)
       clean
  -    generate-cmake-libcxx-win
  +    generate-cmake-libcxx-win -DLIBCXX_TEST_PARAMS="enable_experimental=False"
       echo "+++ Running the libc++ tests"
       ${NINJA} -vC "${BUILD_DIR}" check-cxx
   ;;
  diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
  index 6d878195fb8a..3b860b8538f2 100644
  --- a/libcxx/utils/libcxx/test/params.py
  +++ b/libcxx/utils/libcxx/test/params.py
  @@ -67,7 +67,12 @@ def getUnstableFlag(cfg):
     if hasCompileFlag(cfg, '-funstable') and False: # TODO: Enable this once the design of `-funstable` is finished
       return '-funstable'
     else:
  -    return '-D_LIBCPP_ENABLE_EXPERIMENTAL -lc++experimental'
  +    # When linking in MSVC mode via the Clang driver, a -l<foo>
  +    # maps to <foo>.lib, so we need to use -llibc++experimental here
  +    # to make it link against the static libc++experimental.lib.
  +    # We can't check for the feature 'msvc' in available_features
  +    # as those features are added after processing parameters. 
  +    return '-D_LIBCPP_ENABLE_EXPERIMENTAL ' + ('-llibc++experimental' if _isMSVC(cfg) else '-lc++experimental')
   
   DEFAULT_PARAMETERS = [
     Parameter(name='target_triple', type=str,

The current patch lost the ugly `-llibc++experimental` conditional, which is necessary for it to work in the clang-cl-static config (where libc++experimental is usable). For the clang-cl-dll config, we pretty much need to skip testing c++experimental because it's not usable in that configuration.

(For context, to make libc++experimental work in clang-cl-dll configs; we'd need to split all the `_LIBCPP_TYPE_VIS` and `_LIBCPP_FUNC_VIS` into a separate `_LIBCPP_EXPERIMENTAL_FUNC_VIS`, so that the headers would signal dllimport for the things that are provided by the main shared libc++ library, but signal static linkage for functions/types/templates that are provided by the statically linked libc++experimental. I'm not sure if we're willing to go there?)



================
Comment at: libcxx/utils/libcxx/test/params.py:68
+  if hasCompileFlag(cfg, '-funstable') and False: # TODO: Enable this once the design of `-funstable` is finished
+    return '-funstable'
+  else:
----------------
Actually, I'm not entirely convinced that this is a good way to handle linking against the library for tests.

When building tests, we don't rely on the compiler to implicitly link in our C++ library, but we link with `-nostdlib++` (or `-nodefaultlibs`) and explicitly tell the compiler how to link in specifically the C++ library we've just built (in the test config files).

So in general, if linking with `-nostdlib++ -fexperimental-library` I kinda wouldn't expect the compiler driver to link against the library at all? It's kinda the same as if you'd do `-stdlib=libstdc++ -fexperimental-library` - we can't have that add `-lc++experimental`, as that only makes sense as long as we have `-stdlib=libc++`. So subsequently I don't think the compiler driver should be adding `-lc++experimental` as long as we're passing `-nostdlib++` either?

But if libc++experimental is built unconditionally, wouldn't it be simplest to just always link against it in the test config files (just like how we force it to link against specifically the just-built libc++.a) - that would make it much more symmetrcial to how we handle the main `-lc++`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128927