This is a follow-up for https://reviews.llvm.org/D120160 that addresses some of the post-merge feedback.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Thanks a lot for the fixes @egorzhdan! I think this looks pretty good. Since LLVM 15 is coming up and we'd like to have this in its final state for libc++ purposes, I'll commandeer this to make a couple adjustments and I'll merge this after seeking more feedback on the overall design. Thanks for doing all the work!
Thanks @egorzhdan for working on this, I seem to have overlooked this item in the review queue.
SGMT, but it seems the libc++ CI hasn't been triggered with this change. Maybe change one file in libc++ to give the CI a spin?
clang/docs/ClangCommandLineReference.rst | ||
---|---|---|
272 | I'm not happy with the word "should", I don't want to decide what users should or should not do. Several of these experimental features are perfectly usable. If you can rebuild your entire software stack ABI and API stability might be less of an issue. (The lack of polish of some features might be a reason not to use them.) | |
clang/include/clang/Driver/Options.td | ||
1194 | Likewise regarding should. |
clang/docs/ClangCommandLineReference.rst | ||
---|---|---|
270 | The doc is auto-generated by tablegen (https://discourse.llvm.org/t/clang-driver-options-td-docs-usersmanual-rst-and-docs-clangcommandlinereference-rst/56540). |
- Rebase
- Rename to -fexperimental-library
- Add tests
- Make sure we link against -lc++experimental
clang/docs/ClangCommandLineReference.rst | ||
---|---|---|
270 | Re-generated, thanks. |
Interesting! @urnathan , is there any documentation/discussion on what -std=c++{current,future} would do? I think it's unrelated to this specific proposal, since -fexperimental-library is valid regardless of the Standard mode, however I still think it would make sense to align what Clang and GCC do. One idea we had floated around was -std=c++latest meaning the latest officially ratified Standard, and -std=c++next being the one currently cooking. For example, right now that would give us -std=c++latest == -std=c++20 and -std=c++next == -std=c++2b.
clang/include/clang/Driver/Options.td | ||
---|---|---|
1192–1198 | This can be simplified with OptInCC1FFlag (both driver/CC1 for the pos form, but driver-only for the neg form). | |
clang/lib/Driver/ToolChain.cpp | ||
1016 | There may be an archive ordering problem. | |
clang/lib/Driver/ToolChains/AIX.cpp | ||
281 | There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi | |
clang/lib/Driver/ToolChains/BareMetal.cpp | ||
282 | There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi | |
clang/lib/Driver/ToolChains/CloudABI.cpp | ||
124 | There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi | |
clang/lib/Driver/ToolChains/MipsLinux.cpp | ||
119 | There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi | |
clang/lib/Driver/ToolChains/OpenBSD.cpp | ||
337 | There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi | |
clang/lib/Driver/ToolChains/VEToolchain.cpp | ||
153 | There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi | |
clang/lib/Driver/ToolChains/WebAssembly.cpp | ||
450 | There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi | |
clang/test/Driver/experimental-library-flag.cpp | ||
6 | End full sentences with a period. | |
7 | Suggest testing -lc++ as well |
clang/include/clang/Driver/Options.td | ||
---|---|---|
1192–1198 | I was looking for documentation on OptInCC1FFlag, and I found: // A boolean option which is opt-in in CC1. The positive option exists in CC1 and // Args.hasArg(OPT_ffoo) can be used to check that the flag is enabled. // This is useful if the option is usually disabled. // Use this only when the option cannot be declared via BoolFOption. multiclass OptInCC1FFlag<string name, string pos_prefix, string neg_prefix="", It says to use BoolFOption is we can. So should I stick with BoolFOption? | |
clang/lib/Driver/ToolChain.cpp | ||
1016 | I'm not sure I follow -- what problem are you concerned about? |
The experimental-library-flag test is currently failing on Windows because on Windows, -stdlib=libc++ seems to be ignored and we don't add -lc++ or -lc++experimental. Does someone understand how things are supposed to work when using libc++ on Windows? @mstorsjo maybe?
Clarification - I presume it would fail when targeting MSVC, not when targeting mingw.
Yes, I think -stdlib=libc++ has no effect on MSVC targets currently, and you have to add the include directory and lib arguments when using it - I think @rnk, @hans or @thakis can confirm that.
@phosek has a patch to take the option into use there too, but it's essentially blocked by D103947 if I remember correctly. (The chain of dependencies that trigger that is a bit non-obvious, but I think it was the case that they build their Clang with -DCLANG_DEFAULT_CXX_STDLIB=libc++, and once that has an effect on the MSVC configs, a bunch of code in their build setup suddenly ends up using libc++ instead of MSVC STL, and there's a lot of code there that is built with _HAS_EXCEPTIONS=0 - hence libc++ needing to work in that configuration.)
clang/include/clang/Driver/Options.td | ||
---|---|---|
1192–1198 | OK. Using BoolFOption is fine as we can express the ExperimentalLibrary logic in the tablegen file. I just feel that the boilerplate is a bit higher than OptInCC1FFlag. | |
clang/lib/Driver/ToolChain.cpp | ||
1016 | https://lld.llvm.org/ELF/warn_backrefs.html When these -l options are used to link archives (.a), they should be added in a dependency order. -lc++experimental presumably uses symbols from -lc++abi and must precede -lc++abi. For macOS and Windows, the order usually doesn't matter. |
Awesome, thanks both for the additional context. I'll try to take a look at D103947 soon. In the meantime, I'll mark the test as XFAIL on Windows just to unblock this patch, because it absolutely needs to land for LLVM 15.
clang/lib/Driver/ToolChain.cpp | ||
---|---|---|
1016 | Got it, thanks. In this case, however, we're only linking against libc++, not libc++abi. So presumably there isn't any potential issue here? However, I agree that the other places where I had -lc++experimental after -lc++abi are a problem, and I am fixing those. | |
clang/test/Driver/experimental-library-flag.cpp | ||
7 | IMO this is not the right place to test this -- this should only be testing stuff that is specific to -fexperimental-library. |
Windows seems to have been "fixed" (a band-aid really), and the Linux failure seems unrelated, so I'll ship this now.
I saw that this change broke llvm-clang-x86_64-sie-ubuntu-fast, and I am following up with the bot owner to understand what's special about their target. I'm not being notified of any other failures so far.
I just got notified of a failure on clang-ppc64-aix. I'll take a look first thing tomorrow morning.
The doc is auto-generated by tablegen (https://discourse.llvm.org/t/clang-driver-options-td-docs-usersmanual-rst-and-docs-clangcommandlinereference-rst/56540).
For lengthy documentation, use clang/docs/UsersManual.rst instead