Page MenuHomePhabricator

[libc++] Always build c++experimental.a
ClosedPublic

Authored by ldionne on Jun 30 2022, 9:06 AM.

Details

Summary

This is the first part of a plan to ship experimental features
by default while guarding them behind a compiler flag to avoid
users accidentally depending on them. Subsequent patches will
also encompass incomplete features (such as <format> and <ranges>)
in that categorization. Basically, the idea is that we always
build and ship the c++experimental library, however users can't
use what's in it unless they pass the -funstable flag to Clang.

Note that this patch intentionally does not start guarding
existing <experimental/FOO> content behind the flag, because
that would merely break users that might be relying on such
content being in the headers unconditionally. Instead, we
should start guarding new TSes behind the flag, and get rid
of the existing TSes we have by shipping their Standard
counterpart.

Also, this patch must jump through a few hoops like defining
_LIBCPP_ENABLE_EXPERIMENTAL because we still support compilers
that do not implement -funstable yet.

Diff Detail

Event Timeline

ldionne created this revision.Jun 30 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 9:06 AM
ldionne requested review of this revision.Jun 30 2022, 9:06 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 30 2022, 9:06 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added subscribers: libcxx-commits, Restricted Project, cfe-commits. · View Herald Transcript
ldionne edited the summary of this revision. (Show Details)Jun 30 2022, 9:10 AM

@mstorsjo I might need some assistance figuring out what to do with the Windows parts of this change.

Mordante accepted this revision.Jun 30 2022, 9:39 AM

Thanks for working on this! LGTM with some suggestions.

libcxx/docs/ReleaseNotes.rst
226 ↗(On Diff #441428)

I agree this is a build system change, but I wonder how many users read this part and find the hidden gem.
Should we add a note at new features too? Maybe there we can mention format and ranges are available with this new switch.
(technically that actually belongs in the next patch, but I wouldn't mind to do it in this patch.)

libcxx/docs/UsingLibcxx.rst
42

I think it would be good to explain what experimental features are. The current wording makes it sound the features may not be part of an IS, WP, or TS.

How about something like

Libc++ provides implementations of some experimental features. Experimental features
are either a TS or a feature in the Standard whose implementation is incomplete. Those
features are disabled by default because they are neither API nor ABI stable. However,
the Clang flag ``-funstable`` can be used to turn those features on.
This revision is now accepted and ready to land.Jun 30 2022, 9:39 AM
ldionne updated this revision to Diff 442378.Jul 5 2022, 1:00 PM
ldionne marked 2 inline comments as done.

Address review comments and try to fix some CI issues

libcxx/docs/ReleaseNotes.rst
226 ↗(On Diff #441428)

I added a note to do this in the next patch, since I think it belongs there.

@mstorsjo I might need some assistance figuring out what to do with the Windows parts of this change.

I can try to have a look in a couple days or so - I'm vacationing and have been travelling, but I'm slowly trying to catch up on things here.

Mordante accepted this revision.Jul 6 2022, 11:07 AM

I had a look at the changes and still LGTM!

ldionne updated this revision to Diff 442667.Jul 6 2022, 12:39 PM

Rebase onto main and temporarily disable experimental tests on Windows

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?)

libcxx/utils/libcxx/test/params.py
71

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++?

ldionne marked an inline comment as done.Jul 8 2022, 8:17 AM

Thanks @mstorsjo! Regarding _LIBCPP_EXPERIMENTAL_FUNC_VIS, yes I think it would make sense to use a different visibility macro for symbols that we know are provided only as part of a static library. I would not call it _LIBCPP_EXPERIMENTAL_FUNC_VIS though, I would call it something like _LIBCPP_STATIC_LIBRARY_FUNC_VIS or something like that.

libcxx/utils/libcxx/test/params.py
71

Interesting point. It does mean that we can never rely on -funstable adding -lc++experimental from our test suite -- I think that's OK.

However, I'd like to avoid linking against -lc++experimental in the base test configuration, simply because the base test configuration should mirror the minimal way of invoking Clang to use libc++.

ldionne updated this revision to Diff 443252.Jul 8 2022, 8:18 AM
ldionne marked an inline comment as done.

Windows fixes.

Thanks @mstorsjo! Regarding _LIBCPP_EXPERIMENTAL_FUNC_VIS, yes I think it would make sense to use a different visibility macro for symbols that we know are provided only as part of a static library. I would not call it _LIBCPP_EXPERIMENTAL_FUNC_VIS though, I would call it something like _LIBCPP_STATIC_LIBRARY_FUNC_VIS or something like that.

Sure, _LIBCPP_STATIC_LIBRARY_FUNC_VIS sounds good to me too.

libcxx/utils/libcxx/test/params.py
71

Yep. Also another example of why we can't really rely on the compiler driver adding it - technically I guess the compiler driver might even choose not to pass a generic -lc++ or -lc++experimental, but pass an absolute path to the libc++.{a,so,dylib} that is bundled with the compiler (clang does this for compiler-rt builtins) - so we really do need to rely on -nodefaultlibs and/or -nostdlib++ omitting the compiler-provided defaults.

Keeping adding -lc++experimental here instead of adding it in the base config files is fine with me.

ldionne updated this revision to Diff 443342.Jul 8 2022, 1:45 PM

Rebase onto main and remove references to -funstable since the design is still changing.

This revision was landed with ongoing or failed builds.Jul 8 2022, 1:58 PM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Wed, Jul 13, 9:21 AM

It looks like this is causing macOS builds to fail on GreenDragon. Please take a look and refer the commit if it takes longer to resolve, as the bot has been red for a while now.

[3495/5980] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DBUILD_EXAMPLES -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/projects/libcxx/src -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/libcxx/src -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/include -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers  -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -O2 -g  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.9 -UNDEBUG -faligned-allocation -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-error -std=c++2a -MD -MT projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o -MF projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o.d -o projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o -c /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/libcxx/src/experimental/memory_resource.cpp
FAILED: projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DBUILD_EXAMPLES -D_LIBCPP_BUILDING_LIBRARY -D_LIBCPP_DISABLE_NEW_DELETE_DEFINITIONS -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/projects/libcxx/src -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/libcxx/src -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/include -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers  -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -O2 -g  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.9 -UNDEBUG -faligned-allocation -nostdinc++ -fvisibility-inlines-hidden -fvisibility=hidden -Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -Werror=return-type -Wextra-semi -Wundef -Wformat-nonliteral -Wno-user-defined-literals -Wno-covered-switch-default -Wno-error -std=c++2a -MD -MT projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o -MF projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o.d -o projects/libcxx/src/CMakeFiles/cxx_experimental.dir/experimental/memory_resource.cpp.o -c /Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/libcxx/src/experimental/memory_resource.cpp
/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/libcxx/src/experimental/memory_resource.cpp:9:10: fatal error: 'experimental/memory_resource' file not found
#include <experimental/memory_resource>
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
[3496/5980] /Applications/Xc
ldionne added a comment.EditedWed, Jul 13, 9:28 AM

It looks like this is causing macOS builds to fail on GreenDragon. Please take a look and refer the commit if it takes longer to resolve, as the bot has been red for a while now.

Thanks for the heads up. Those bots shouldn't be building libc++ at all. Let's take that offline.

Edit: CF https://github.com/llvm/llvm-zorg/pull/28

hans added a subscriber: hans.Mon, Jul 18, 1:50 AM

It looks like this is causing macOS builds to fail on GreenDragon. Please take a look and refer the commit if it takes longer to resolve, as the bot has been red for a while now.

Thanks for the heads up. Those bots shouldn't be building libc++ at all. Let's take that offline.

Edit: CF https://github.com/llvm/llvm-zorg/pull/28

We're hitting the same "memory_resource.cpp:9:10: fatal error: 'experimental/memory_resource' file not found" error in Chromium (http://crbug.com/1345033).

The file exists, so the error is surprising. Since the GreenDragon bots also hit this, it seems there's a bug here that needs fixing?

hans added a comment.Mon, Jul 18, 4:56 AM

After some more digging, it seems we're hitting the same issue as we did back in https://reviews.llvm.org/D82702#2153130 and now the work-around of passing -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF stopped working.

hans added a subscriber: thakis.Mon, Jul 18, 7:58 AM

After discussing with @thakis a bit, I've reverted this in 09cebfb978def7fa2a4460bca89690f8d3608216 to bring builds back to a green state while we figure out what's going on and how to fix this.

The fact that Greendragon broke (example build) seems like a strong indication that this would break others as well, and removing libc++ from those bots seems like avoiding rather than fixing the problem.

If it's the case that Clang and libc++ should no longer be built together (as indicated on https://github.com/llvm/llvm-zorg/pull/28) I think we need a better failure mode than "fatal error: 'experimental/memory_resource' file not found", some good documentation updates to explain what people should be doing instead, and time for people to fix their builds.

As for Chromium, we've been using the technique suggested by @ldionne in https://bugs.chromium.org/p/chromium/issues/detail?id=1067216#c7 plus -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF. We would be grateful for ideas of what we should do instead moving forward.

(Also, this is probably a stupid question, but why can't this library use its own headers? I really don't understand the failure mode here to begin with.)

The weird part here is that you're configuring libc++, but you are building neither the static nor the shared library. I don't understand why you do that, and that may hide some other more fundamental issue in your setup. Anyways, I think the issue should be resolved by 1d0f79558ca47f50119fb38c62ff5afd3160d260.

ldionne reopened this revision.Tue, Jul 19, 7:43 AM
This revision is now accepted and ready to land.Tue, Jul 19, 7:43 AM
This revision was automatically updated to reflect the committed changes.
hans added a comment.Wed, Jul 20, 7:41 AM

The weird part here is that you're configuring libc++, but you are building neither the static nor the shared library. I don't understand why you do that, and that may hide some other more fundamental issue in your setup.

Yes, I wish we weren't weird.
The reason libc++ is part of our build is that we want the headers so that our newly built Clang will be able to build c++ files. https://bugs.chromium.org/p/chromium/issues/detail?id=1067216#c7 has the background for our current situation.

In https://github.com/llvm/llvm-zorg/pull/28 you mentioned that "for a couple of years now, Clang and libc++ are entirely separate projects w.r.t. how they are shipped on Apple platforms, and it doesn't make sense to build libc++ at the same time as Clang anymore".
Does that mean the libc++ headers have moved to the SDK now, and if so from what version? If it's in the SDK version we use (I'd have to check), perhaps that would allow us to stop building like this.

Anyways, I think the issue should be resolved by 1d0f79558ca47f50119fb38c62ff5afd3160d260.

Yes, that fixed it for us. Thanks!

The weird part here is that you're configuring libc++, but you are building neither the static nor the shared library. I don't understand why you do that, and that may hide some other more fundamental issue in your setup.

Yes, I wish we weren't weird.
The reason libc++ is part of our build is that we want the headers so that our newly built Clang will be able to build c++ files. https://bugs.chromium.org/p/chromium/issues/detail?id=1067216#c7 has the background for our current situation.

In https://github.com/llvm/llvm-zorg/pull/28 you mentioned that "for a couple of years now, Clang and libc++ are entirely separate projects w.r.t. how they are shipped on Apple platforms, and it doesn't make sense to build libc++ at the same time as Clang anymore".
Does that mean the libc++ headers have moved to the SDK now, and if so from what version? If it's in the SDK version we use (I'd have to check), perhaps that would allow us to stop building like this.

Yes, the libc++ headers are in the SDK. They have been since Xcode 12 if I'm not mistaken. They are *also* still shipped in the Clang toolchain, but that will change eventually -- there are a few things blocking us from doing that, but they will go away eventually.

hans added a comment.Fri, Jul 22, 6:34 AM

In https://github.com/llvm/llvm-zorg/pull/28 you mentioned that "for a couple of years now, Clang and libc++ are entirely separate projects w.r.t. how they are shipped on Apple platforms, and it doesn't make sense to build libc++ at the same time as Clang anymore".
Does that mean the libc++ headers have moved to the SDK now, and if so from what version? If it's in the SDK version we use (I'd have to check), perhaps that would allow us to stop building like this.

Yes, the libc++ headers are in the SDK. They have been since Xcode 12 if I'm not mistaken. They are *also* still shipped in the Clang toolchain, but that will change eventually -- there are a few things blocking us from doing that, but they will go away eventually.

We've removed libc++ from our toolchain build process (https://chromium-review.googlesource.com/c/chromium/src/+/3779521) so hopefully will see fewer problems of this nature in the future. Thanks!