[libcxx-commits] [PATCH] D68805: [libcxx] Remove shared_ptr::make_shared

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a subscriber: Bigcheese.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/memory:4421
+
+    return shared_ptr<_Tp>::__create_with_cntrl_block(__hold2.get()->get(),
+                                                      __hold2.release());
----------------
zoecarver wrote:
> ldionne wrote:
> > 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.
> 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. 
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68805