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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

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

> > 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'm not talking about `reset`. I'm saying inline `reset` into the destructor and then exit early. Basically, replace the destructor with this:
>
>   ~unique_ptr() {
>       pointer __tmp = __ptr_.first();
>       if (!__tmp)
>           return;
>       __ptr_.first() = pointer(); // pointer() = nullptr
>       __ptr_.second()(__tmp);
>   }
>


Ok, got it. Alternatively:

  ~unique_ptr() {
      pointer __tmp = __ptr_.first();
      if (__tmp) {
          __ptr_.first() = pointer();
          __ptr_.second()(__tmp);
      }
  }

Yeah, I guess this gets rid of an assignment to the pointer when it's null. But really, at that point we're not getting a lot of bang for the buck. I'd rather leave the implementation as `reset()`, or acknowledge the more important issue that we're setting something that's about to die, and use your original implementation.

For the time being, I'd be tempted to do nothing until we have a better way to enable assertions in libc++. If we change this and it causes even just one security vulnerability, I think we'll be losing overall because the perf benefits are most likely non-existent (at the very least the bug reporter in PR45848 doesn't prove there's a slowdown in a very convincing way IMO).

Once we have a hardened mode for libc++, we can revisit this issue and avoid setting the pointer in the normal mode. Doing so might even make it easier to find problems with ubsan.

@atmnpatel @zoecarver WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80057