Page MenuHomePhabricator

[libc++] Rename *SAFE_STATIC to *CONSTINIT, and normalize its uses.
ClosedPublic

Authored by Quuxplusone on Feb 8 2022, 10:41 AM.

Details

Reviewers
ldionne
Mordante
Group Reviewers
Restricted Project
Restricted Project
Commits
rG05337a756c66: [libc++] Rename *SAFE_STATIC to *CONSTINIT, and normalize its uses.
Summary

As I suggested in D118874.
This is not NFC; if we're worried about the functional changes, I'm willing to split this into an NFC and a cleanup.

In particular I'm not clear on whether we can safely assume, inside src/*.cpp:

  • that C++11 is available? Several places use 0 for nullptr, and _LIBCPP_CONSTEXPR for constexpr. Can I fix those?
  • that C++20 is available? Could we just use constinit directly instead of _LIBCPP_CONSTINIT, in src/*.cpp?

In the .h files we're at the mercy of the user-programmer and must generally assume C++03, but in src/*.cpp we (that is, our vendors) control the compiler switches.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Feb 8 2022, 10:41 AM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 8 2022, 10:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
libcxx/src/experimental/memory_resource_init_helper.h
2

Here I wonder if adding static would be helpful. (This is included from within an anonymous namespace already, so it'd be redundant to the computer, but possibly helpful to the human reader.)

libcxx/src/memory.cpp
135–136

Here I'm changing constinit const to constexpr. I suspect the original code was just static const (i.e., C++03-friendly constexpr), and then _LIBCPP_SAFE_STATIC got added basically in a cut-and-paste error.

174

Here I'm adding constinit to this array that seems like it was always intended to be constinit. (The compiler optimizer was probably already doing the obvious thing anyway.)

libcxxabi/src/cxa_default_handlers.cpp
108

Here's a place it would be nice to use nullptr, if it's allowed.

Remove some more redundant constexpr-friendliness declarations in the coroutines tests.
Boldly use nullptr in the place I said it'd be nice to use it; let's see if CI is happy with it.

ldionne accepted this revision.Feb 8 2022, 2:08 PM

LGTM in essence, however some tests are failing. Also, we are free to assume C++20 the we build the library (in src/), because we require it here: https://github.com/llvm/llvm-project/blob/main/libcxx/CMakeLists.txt#L538-L541.

Ship it when this passes, and I suggest you use constinit directly in src/.

This revision is now accepted and ready to land.Feb 8 2022, 2:08 PM
Mordante accepted this revision.Feb 9 2022, 8:23 AM

Thanks for the cleanup! LGTM!

libcxx/src/debug.cpp
37–38

As @ldionne already pointed out we require C++20 to build the dylib so I would suggest to use constinit here.

Fix the failing tests (forgot to remove a use of the removed constinit variable), and use constinit directly in libcxx/src/.
Should I use constinit directly in libcxxabi/src/ too? So far I have not.

Fix the failing tests (forgot to remove a use of the removed constinit variable), and use constinit directly in libcxx/src/.
Should I use constinit directly in libcxxabi/src/ too? So far I have not.

I think you can, since we require CXX20 from the libc++abi CMake too. I think I checked yesterday and that was the case. If you try and it fails while building libc++abi, you'll have your answer.

This is why keeping compiler requirements fairly up-to-date is nice -- we don't have to jump through too many hoops in the sources at least.

Use constinit in libcxxabi/src/*.cpp, too.

CI failures: https://buildkite.com/llvm-project/libcxx-ci/builds/8602
Roll back some of the libcxxabi/src/ modernizations.

In src/, most files can use `constinit` directly because they're always
compiled with C++20. But some files, like "libcxxabi/src/fallback_malloc.cpp",
can't, because they're `#include`d directly from test cases in libcxxabi/test/
and therefore must (currently) compile as C++03. We might consider refactoring
those offending tests, or at least marking them `UNSUPPORTED: c++03`.

Let's ship this!

This revision was landed with ongoing or failed builds.Feb 15 2022, 7:41 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Feb 17 2022, 5:42 AM

Hi, this is causing us some trouble: We build with a very new libc++abi (our project autoupdates to the latest version of libc++abi every time a change lands), but an old-ish (1 year maybe) libc++ (our libc++ doesn't autoupdate yet – we're working on that part).

So our libc++ header files don't have _LIBCPP_CONSTINIT yet (it's added in this change here), but libc++abi requires it now.

Even if our libc++ would autoupdate, we probably wouldn't be able to auto-import this change automatically since libc++ and libc++abi would still autoupdate separately from each other.

If this is something you want to address at all is up to you – our setup is admittedly weird, but libc++ and libc++abi _are_ separate projects, and our setup has been working fine for the last few years.

If you do want to address it, the easiest fix is probably to just use constinit instead of _LIBCPP_CONSTINIT in libc++abi (and disable the tests that include the internal header, or make them build with -std=c++20 too, or something).

Thanks for your consideration.

thakis added a subscriber: hans.Feb 17 2022, 5:43 AM

@hans had the good idea that we can build libc++abi with -D_LIBCPP_CONSTINIT=constinit for now, which unblocks us for now. It'd still be nice if we didn't have to do that though :)

If you do want to address it, the easiest fix is probably to just use constinit instead of _LIBCPP_CONSTINIT in libc++abi (and disable the tests that include the internal header, or make them build with -std=c++20 too, or something).

If we want to address it, I'd suggest we could add

#ifndef _LIBCPP_CONSTINIT
#define _LIBCPP_CONSTINIT constinit
#endif

somewhere near the top of the offending files.

But I'll defer to @ldionne as to whether we want to address this at all; I have no opinion myself, and it sounds like you found an okay workaround. @ldionne?