Page MenuHomePhabricator

[libc++] Always query the compiler to find whether a type is always lockfree
ClosedPublic

Authored by ldionne on Tue, Sep 6, 2:27 PM.

Details

Summary

In https://llvm.org/D56913, we added an emulation for the __atomic_always_lock_free
compiler builtin when compiling in Freestanding mode. However, the emulation
did (and could not) give exactly the same answer as the compiler builtin,
which led to a potential ABI break for e.g. enum classes.

After speaking to the original author of D56913, we agree that the correct
behavior is to instead always use the compiler builtin, since that provides
a more accurate answer, and __atomic_always_lock_free is a purely front-end
builtin which doesn't require any runtime support. Furthermore, it is
available regardless of the Standard mode (see https://godbolt.org/z/cazf3ssYY).

However, this patch does constitute an ABI break. As shown by https://godbolt.org/z/1eoex6zdK:

  • In LLVM <= 11.0.1, an atomic<enum class with 1 byte> would not contain a lock byte.
  • In LLVM >= 12.0.0, an atomic<enum class with 1 byte> would contain a lock byte.

This patch breaks the ABI again to bring it back to 1 byte, which seems
like the correct thing to do.

Fixes #57440

Diff Detail

Event Timeline

ldionne created this revision.Tue, Sep 6, 2:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 6, 2:27 PM
ldionne requested review of this revision.Tue, Sep 6, 2:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 6, 2:27 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: thieta.Tue, Sep 6, 2:29 PM

This is for initial comments. I will figure out a way to test this tomorrow. We'll also need to discuss whether we should cherry-pick to LLVM 15.

I think we should, since we should fix the ABI break as soon as feasible. @thieta Any opinion? FWIW, this only concerns freestanding users, so this is not as widespread as it may seem at first sight.

ldionne updated this revision to Diff 458460.Wed, Sep 7, 8:13 AM

Add tests.

thieta added a comment.Wed, Sep 7, 8:55 AM

When you say freestanding: what does that mean? Ping-pong in the ABI is never great - but if it's not for all users maybe it's worth doing if we think it'll be stable going forward. Do you think it would warrant a 15.1.0 instead of a 15.0.1 in that case?

Thanks for picking this up! In general happy with it, but some minor issue.

libcxx/include/atomic
1112

Can you update the release notes for this change.

1409

Can you add a comment why we use the C++17 feature here unconditionally?
This looks like a bug and surely somebody will try to fix it in 5 years time ;-)

ldionne marked an inline comment as done.Thu, Sep 15, 10:50 AM

When you say freestanding: what does that mean?

I mean users literally passing the -ffreestanding flag. This is used on embedded platforms, and it's generally a much smaller audience than the mainstream development communities. Not to downplay the importance of it at all, quite the opposite, however in terms of impact this should reach much fewer developers.

Ping-pong in the ABI is never great - but if it's not for all users maybe it's worth doing if we think it'll be stable going forward. Do you think it would warrant a 15.1.0 instead of a 15.0.1 in that case?

Well, I guess the situation is really that we introduced a bug which made our ABI very sub-optimal (i.e. there's a lock in the atomic even when it could be lock free). I think it is definitely worth solving. I don't think it is necessary to do a 15.1.0, I'll just call it out in the release notes.

libcxx/include/atomic
1409

I am not sure why it looks like a bug? I'll add a comment to the definition of __libcpp_is_always_lock_free explaining that the compiler builtin we're using there is available in all versions.

ldionne updated this revision to Diff 460458.Thu, Sep 15, 10:50 AM
ldionne marked 2 inline comments as done.

Address comments.

ldionne accepted this revision.Mon, Sep 19, 8:09 AM
This revision is now accepted and ready to land.Mon, Sep 19, 8:09 AM