[libcxx-commits] [PATCH] D80057: [libc++][NFCI] Optimization to std::~unique_ptr

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

In D80057#2049216 <https://reviews.llvm.org/D80057#2049216>, @zoecarver wrote:

> @ldionne
>
> > In your example, I don't see what __p is.
>
> `__p` is `nullptr`. I still think this might be a good solution. Essentially exiting `reset` early if `__ptr` is null.


We can't do that, because `p.reset(x)` needs to set `p.__ptr_` to `x`, regardless of whether `p.__ptr_` is `null` or not. Or are we talking past each other?

> [...]
>  I think you'd need to create a lot of null unique_ptrs.

As you mentioned, there's a dead store whenever the `unique_ptr` escapes, so it's more than just null `unique_ptr`s. However, since it's in the destructor of the object, why does it matter whether the `unique_ptr` has escaped? If the answer is "it doesn't matter", then I think we could improve the codegen here such that the dead store is always removed. Naively, I would certainly have expected that the dead store is always removed, regardless of escaping.

However, that change would have the same security implications, except even wider because it would impact all code, not only `~unique_ptr()`. @jfb Would you too expect the dead store to always be removed?

In D80057#2049148 <https://reviews.llvm.org/D80057#2049148>, @jfb wrote:

> Fine by me as long as it's heavily publicized, so folks realize what's being changed.


We can add a release note. @atmnpatel can you add one to `libcxx/docs/ReleaseNotes.rst`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80057