Page MenuHomePhabricator

[libc++] [ABI BREAK] Remove non-atomic "platform" semaphore implementations.
ClosedPublic

Authored by Quuxplusone on Sep 20 2021, 3:08 PM.

Details

Summary
These can't be made constexpr-constructible (constinit'able),
so they aren't C++20-conforming. Also, the platform versions are
going to be bigger than the atomic/futex version, so we'd have
the awkward situation that `semaphore<42>` could be bigger than
`semaphore<43>`, and that's just silly.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Sep 20 2021, 3:08 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptSep 20 2021, 3:08 PM

Rebase onto D110042. Enable the unit test for constinit-friendliness.

Poke buildbot after landing D110042.

On the one hand, the platform semaphore is going to work better because it uses the OS primitives, so that will respect whatever implementation is preferred on the OS. On the other hand, I find it surprising that the constructor is marked as constexpr if that can't be implemented using the most common implementation, i.e. pthread's sem_init. I'd like to see what @__simt__ or @wash have to say about this.

Hmm. I lost track of how/when this constructor got defined as constexpr. It doesn't mean that it's unimplementable with platform semaphores, but to do so would make the implementation a lot more complicated, slower, and the object even larger than it already is.

I note that the MSVC STL also implemented semaphores exclusively on atomics, like this.

I'm personally pretty comfortable with this change.

ldionne accepted this revision.Oct 21 2021, 6:53 AM

@Quuxplusone Please add a release note mentioning that we broke the ABI of counting_semaphore and ship it. Once you do, let me know and I'll cherry-pick to the release/13 branch.

This revision is now accepted and ready to land.Oct 21 2021, 6:53 AM

Add a release note. @ldionne, is this what you had in mind? I'm not clear on the procedure for release-noting things in general, let alone something that we're changing (I presume) between 13.0 and 13.1 — should that even get a 14.x release note? Anyway, please take a look and let me know before I'll land this in main. (Or just land this yourself if it looks good.)

Quuxplusone edited the summary of this revision. (Show Details)Oct 21 2021, 5:50 PM

Rebased on main.
@ldionne ping! do you want a release note in 14.x or just when you put it in 13.x? is this a good release note?

Rebased on main.
@ldionne ping! do you want a release note in 14.x or just when you put it in 13.x? is this a good release note?

I'll add the release note to 13.x when I cherry-pick it manually.

libcxx/docs/ReleaseNotes.rst
89 ↗(On Diff #383388)

Heads up @tstellar I'd like to cherry-pick this change to 13.x to minimize the impact of the ABI break (the sooner we release it, the less time folks have to ship code that would get bitten by the change).

Copy UNSUPPORTED: linux && 32bits-on-64bits annotations from the other semaphore tests to deal with the failures in https://buildkite.com/llvm-project/libcxx-ci/builds/6278 . No idea why these are newly failing after this PR (as opposed to having already been failing when Louis annotated literally all the other semaphore tests).

If this passes CI, I'll go ahead and land it.

libcxx/docs/ReleaseNotes.rst
89 ↗(On Diff #383388)

IIUC, you want me to eliminate the diff in ReleaseNotes.rst from this PR, and then you'll add it manually when you cherry-pick. The diff you'll add manually will be (mildly edited-yet-again by me):

- The C++20 type ``std::counted_semaphore<N>`` is now based on ``std::atomic``
  on all platforms, and does not use "native" semaphores such as pthreads
  ``sem_t`` even on platforms that would support them. This changes the layout
  of ``counted_semaphore<N>``, so it is an ABI break. This change is needed to
  conform to the Standard, which requires ``counted_semaphore``'s constructor
  to be constexpr.
ldionne accepted this revision.Thu, Nov 4, 11:18 AM

LGTM. Yes, indeed, I will add that release note to LLVM 13 myself.