This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Remove shared_ptr::make_shared

Authored by zoecarver on Oct 10 2019, 8:56 AM.



This patch removes shared_ptr::make_shared as it is not part of the standard. This patch also adds __create_with_cntrl_block, which is a helper function that can be used in std::allocate_shared and std::make_shared. This is the third patch (out of 4) from D66178.

Event Timeline

zoecarver created this revision.Oct 10 2019, 8:56 AM
zoecarver edited the summary of this revision. (Show Details)Oct 10 2019, 9:05 AM
ldionne requested changes to this revision.Oct 16 2019, 7:52 AM
ldionne added a subscriber: phosek.

Can you confirm this is the bit of the patch that we suspect had caused a SEGFAULT for @phosek?

Also, is there a reason why we can't just rename shared_ptr<_Tp>::make_shared to shared_ptr<_Tp>::__make_shared, without changing the logic and the signature? Do you have other changes pending that depend on that?

Note: Requesting changes so it shows up properly in my review queue.

This revision now requires changes to proceed.Oct 16 2019, 7:52 AM

@ldionne, yes. I suspect this is the patch that will cause a SEGFAULT. It could also be a similar part of the patch where I remove std::shared_ptr::allocate_shared, though. If it does cause a SEGFAULT we will know exactly where the issue is and it will be easier to debug.

We could mangle the function. But, part of the reason for this change is removing as much code from make_shared as possible so, later, when I add more overloads, there will be less duplication. Additionally, I don't think it makes sense to add an extra layer of abstraction when it gives us no benefit. If we don't want __create_with_cntrl_block I would still like to remove std::shared_ptr::make_shared (and std::shared_ptr::__make_shared).

D62641 depends on this functionality but, it shouldn't be too hard to update that patch (and I wouldn't mind doing it if you want me to remove __create_with_cntrl_block).

ldionne added inline comments.Oct 16 2019, 9:08 AM

Can we call this __create_with_control_block instead? We're not saving many characters.


This just jumped in my face -- it's obvious now.

We're calling a function f(p.get()->foobar(), p.release()) where p is a unique_ptr. If the second argument (p.release()) is evaluated before the first one, p.get() will be nullptr and we'll dereference that, causing the segfault. @phosek must have been testing on a target where the compiler evaluates arguments from right to left.

I think we pinned down the order of evaluation in C++20 (I'm not sure whether it actually got in), but it's definitely a problem for earlier standards anyway.

zoecarver marked 2 inline comments as done.Oct 20 2019, 9:03 AM
zoecarver added inline comments.

Will do.


That would explain it! I'll change this to make __hold2.get()->get() a variable.

I am surprised that compilers are allowed to mess with order of evaluation. Glad it's fixed in C++20, though.

  • rename __create_with_cntrl_block to __create_with_control_block.
  • pull __ptr argument into a variable so that the arguments can be evaluated from right to left without a segfault.
ldionne accepted this revision.Oct 21 2019, 4:42 PM
ldionne added a subscriber: Bigcheese.
ldionne added inline comments.

I am surprised that compilers are allowed to mess with order of evaluation.

So, actually, I just got confirmation from @Bigcheese that there was a proposal to make evaluation of arguments be from left to right, but it didn't go through. Instead, the order moved from unsequenced to indeterminately sequenced in C++17.

This means that evaluation of arguments can't be interleaved, but you still can't rely on any given order.

This revision is now accepted and ready to land.Oct 21 2019, 4:42 PM
zoecarver marked an inline comment as done.Oct 21 2019, 6:57 PM

I'll commit this in the morning.


Good to know; thanks for sharing :)

zoecarver closed this revision.Oct 22 2019, 8:20 AM

Resolved by rL375504.