This is an archive of the discontinued LLVM Phabricator instance.

[libc++] fix `shared_ptr`'s incorrect constraints
ClosedPublic

Authored by huixie90 on Feb 5 2023, 11:31 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Commits
rGa38a4654ce4b: [libc++] fix `shared_ptr`'s incorrect constraints
Summary

Fix several bugs:

  1. https://llvm.org/PR60258 The conversion constructors' constraint __compatible_with incorrectly allow array types conversion to scalar types
  2. https://llvm.org/PR53368 The constructor that takes unique_ptr are not suffiently constrained.
  3. The constructors that take raw pointers incorretly use __compatible_with. They have different constraints

Diff Detail

Event Timeline

huixie90 created this revision.Feb 5 2023, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2023, 11:31 AM
huixie90 requested review of this revision.Feb 5 2023, 11:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2023, 11:31 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
huixie90 updated this revision to Diff 494934.Feb 5 2023, 11:35 AM

update commit msg

huixie90 edited the summary of this revision. (Show Details)Feb 5 2023, 11:35 AM
ldionne accepted this revision.Feb 8 2023, 1:32 PM
ldionne added a subscriber: Restricted Project.

Thanks for fixing this! Only nits.

Let's land this and cherry-pick it onto LLVM 16. Please add a release note on the LLVM 16 release branch to explain that this constrains std::shared_ptr more than it used to, which could lead to a bit of source break. I don't think this should be too big of a deal, especially since the code this would break is arguably brittle.

Pinging vendors for awareness.

libcxx/include/__memory/shared_ptr.h
373
374–380

Along with (optionally) a test case for why this can't be implemented by inheriting from _BoolConstant.

382

Same here, let's use _Tp instead of _Tp2.

391
791

Can you double-check whether this should be element_type or _Tp?

This revision is now accepted and ready to land.Feb 8 2023, 1:32 PM

Thanks for fixing this! Only nits.

Let's land this and cherry-pick it onto LLVM 16. Please add a release note on the LLVM 16 release branch to explain that this constrains std::shared_ptr more than it used to, which could lead to a bit of source break. I don't think this should be too big of a deal, especially since the code this would break is arguably brittle.

Pinging vendors for awareness.

Thanks for the ping, @ldionne. As usual, this causes a few breakages for us, but they're all trivial to fix. No concerns from me to land this whenever you're ready.

Thanks for fixing this! Only nits.

Let's land this and cherry-pick it onto LLVM 16. Please add a release note on the LLVM 16 release branch to explain that this constrains std::shared_ptr more than it used to, which could lead to a bit of source break. I don't think this should be too big of a deal, especially since the code this would break is arguably brittle.

Pinging vendors for awareness.

Thanks for the ping, @ldionne. As usual, this causes a few breakages for us, but they're all trivial to fix. No concerns from me to land this whenever you're ready.

Thanks a lot for checking!

huixie90 updated this revision to Diff 496687.Feb 11 2023, 7:32 AM
huixie90 marked 5 inline comments as done.

address comments

This revision was automatically updated to reflect the committed changes.