This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Add `-fexperimental-library` flag to enable unstable and experimental features: follow-up fixes
ClosedPublic

Authored by ldionne on Mar 7 2022, 11:35 AM.

Diff Detail

Event Timeline

egorzhdan created this revision.Mar 7 2022, 11:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 11:35 AM
Herald added a subscriber: dang. · View Herald Transcript
egorzhdan requested review of this revision.Mar 7 2022, 11:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 11:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ldionne commandeered this revision.Jun 30 2022, 7:13 AM
ldionne edited reviewers, added: egorzhdan; removed: ldionne.

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
1188

Likewise regarding should.

ldionne marked 2 inline comments as done.Jul 8 2022, 6:39 AM
MaskRay added inline comments.Jul 8 2022, 10:00 AM
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).
For lengthy documentation, use clang/docs/UsersManual.rst instead

ldionne updated this revision to Diff 444709.Jul 14 2022, 10:19 AM
ldionne marked an inline comment as done.
  • Rebase
  • Rename to -fexperimental-library
  • Add tests
  • Make sure we link against -lc++experimental
ldionne added inline comments.Jul 14 2022, 10:19 AM
clang/docs/ClangCommandLineReference.rst
270

Re-generated, thanks.

LGTM.

It may be related, @urnathan wants to add -std=c++{current,future} to GCC and may have opinions on the option name.

clang/lib/Driver/ToolChains/Clang.cpp
5840

Use Args.AddLastArg

ldionne marked an inline comment as done.Jul 14 2022, 11:52 AM

LGTM.

It may be related, @urnathan wants to add -std=c++{current,future} to GCC and may have opinions on the option name.

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.

ldionne updated this revision to Diff 444753.Jul 14 2022, 11:52 AM

Address review comment.

MaskRay accepted this revision.Jul 14 2022, 12:02 PM
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
1186

This can be simplified with OptInCC1FFlag (both driver/CC1 for the pos form, but driver-only for the neg form).
You'll need to set CoreOption to make the option available to clang-cl.

clang/lib/Driver/ToolChain.cpp
1016

There may be an archive ordering problem.

clang/lib/Driver/ToolChains/AIX.cpp
279

There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi

clang/lib/Driver/ToolChains/BareMetal.cpp
280

There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi

clang/lib/Driver/ToolChains/CloudABI.cpp
122

There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi

clang/lib/Driver/ToolChains/MipsLinux.cpp
117

There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi

clang/lib/Driver/ToolChains/OpenBSD.cpp
335

There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi

clang/lib/Driver/ToolChains/VEToolchain.cpp
151

There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi

clang/lib/Driver/ToolChains/WebAssembly.cpp
448

There may be an archive ordering problem. -lc++experimental should probably be before -lc++abi

clang/test/Driver/experimental-library-flag.cpp
5

End full sentences with a period.

6

Suggest testing -lc++ as well

This revision is now accepted and ready to land.Jul 14 2022, 12:02 PM
Mordante accepted this revision.Jul 15 2022, 8:29 AM

LGTM, but please update the name of the flag in the title before landing.

ldionne added inline comments.Jul 15 2022, 8:44 AM
clang/include/clang/Driver/Options.td
1186

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?

ldionne retitled this revision from [Clang] Add `-funstable` flag to enable unstable and experimental features: follow-up fixes to [Clang] Add `-fexperimental-library` flag to enable unstable and experimental features: follow-up fixes.Jul 15 2022, 8:51 AM
ldionne added a subscriber: mstorsjo.

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?

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

MaskRay added inline comments.Jul 16 2022, 12:19 AM
clang/include/clang/Driver/Options.td
1186

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.
-lc++abi uses symbols from -lunwind and must precede -lunwind.

For macOS and Windows, the order usually doesn't matter.

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

That's correct, I implemented the support for -stdlib=libc++ on MSVC targets in D101479, but that change caused a number of build failures including the compiler-rt due to _HAS_EXCEPTIONS=0 so relanding that change is blocked on D103947.

ldionne marked 12 inline comments as done.Jul 19 2022, 9:44 AM

That's correct, I implemented the support for -stdlib=libc++ on MSVC targets in D101479, but that change caused a number of build failures including the compiler-rt due to _HAS_EXCEPTIONS=0 so relanding that change is blocked on D103947.

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
6

IMO this is not the right place to test this -- this should only be testing stuff that is specific to -fexperimental-library.

ldionne updated this revision to Diff 445859.Jul 19 2022, 9:48 AM
ldionne marked 2 inline comments as done.

Address most review feedback.

MaskRay accepted this revision.Jul 19 2022, 10:16 AM

Windows seems to have been "fixed" (a band-aid really), and the Linux failure seems unrelated, so I'll ship this now.

This revision was landed with ongoing or failed builds.Jul 19 2022, 12:05 PM
This revision was automatically updated to reflect the committed changes.

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.

clang/test/Driver/experimental-library-flag.cpp