[libcxx-commits] [PATCH] D68480: Implementation of C++20's P1135R6 for libcxx

Sun Jan 2 03:21:16 PST 2022

theraven added a comment.

In D68480#3179526 <https://reviews.llvm.org/D68480#3179526>, @ldionne wrote:

> In D68480#3179355 <https://reviews.llvm.org/D68480#3179355>, @theraven wrote:
>
>> This appears to have baked in some ABI details that don't permit efficient implementation on some of our supported platforms.
>
> This is unfortunate. Have you folks shipped this yet? Perhaps it's not too late to fix things now if you've only released it very recently.

I am not sure who you mean when you say 'you' here.  I am writing this as:

- A member of the LLVM project and libc++ contributor
- A libc++ consumer.

Since Howard's initial commit, libc++ has had strong ABI backwards compatibility guarantees for anything outside of the `experimental` namespace.  Any time we define an interface between the library and the headers, that is an ABI that we need to support in the long term.

I came to this particular patch because one of my colleagues pointed me at `atomic_wait` as a possible replacement for platform-specific wrappers around futex-like abstractions in our code.  I then tried to add FreeBSD and Windows support (the two platforms I care about for our code that do not have platform-specific code paths in libc++) and discovered that the ABI that we have committed to supporting cannot be implemented *at all* on 32-bit architectures with FreeBSD and cannot expose all of the functionality on Windows.

This is because this patch set decided to add a new pattern, rather than copying the same approach that every other atomic op uses: provide `_1`, `_2`, `_4`, `_8`, `_16` and `_n` implementations.  This also means that if Linux adds a `futex64` system call (which I consider fairly probable, given that it is painful on Linux that you can't use a futex with pointers currently) then we can't take advantage of it without an ABI break.

>> In the future, please can you post an RFC from things that need to integrate with platform-specific code? We're now stuck with this interface until we are willing to do an ABI-breaking change.
>
> With all due respect, I consider that the responsibility is yours. In May 2019, Olivier started a thread where he was gathering feedback on what was to become the synchronization library: https://lists.llvm.org/pipermail/libcxx-dev/2019-May/000396.html. The thread got very little traction (see the June 2019 archive for the only few replies). Then, this patch was created and it was under review for roughly 6 months. Anyone paying attention to libc++ development could have seen this go by.

The subject of that thread does not in any way mention that this requires platform-specific logic.  Putting 'platform-specific futex-like' or similar in the subject would have made people pay attention.  No one was tagged as a reviewer here to request perspectives from other platforms (dim or joerg, for example).  It is the responsibility of the committer to ensure that a patch has adequate reviews.  This did not happen here and we are now stuck with this ABI.  I don't know how we can fix this, without adding a new version in the `__2` namespace and eventually having a SONAME bump.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68480/new/

https://reviews.llvm.org/D68480