[libcxx-commits] [PATCH] D130330: [libc++][NFC] Add checks for lifetime issues in classic algorithms.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

In D130330#3671468 <https://reviews.llvm.org/D130330#3671468>, @huixie90 wrote:

> I think this is good. But I'd like to clarify the idea behind it.  IIUC, the idea is that
>
> - there is a global object pool of ptrs
> - the class's destructor removes this from the global pool
> - every operation assert this is inside the pool
>
> But if the object destructor has been run, it is UB to call its member function. and since it is UB , those `assert` in theory is not guaranteed to run

Yes, it's true that this relies on undefined behavior. However, I think it's the pragmatic thing to do:

- I can't think of a way to do this that avoids UB, because checking an object after its destructor has run is really the crux of the problem. `constexpr` checks are great in that regard but unfortunately won't give us full coverage as I mentioned in another comment. So it looks like our alternatives are either having checks that rely on UB or no checks at all (for some cases, like most of `std::sort` which is the motivating example);
- I have manually confirmed that it works (in the sense that it does find actual lifetime issues). It would have found both the original bug that triggered the assertion in Chromium and the fact that the first patch with the fix still contained a dangling temporary;
- I have deliberately written the lifetime checks so that they never dereference `this` (that's one of the reasons the cache is a static variable). In practice, I presume that member function calls translate to regular function calls with `this` as the first parameter (simplifying a little). Neither the function code nor the _value_ of the `this` pointer have a reason to become invalid after the destructor has run. Now, it's possible that some compiler optimization simply prevents the member function from being called since that is supposed to be impossible (for a valid program). I'm very skeptical this could happen in practice, and even if it does, it seems like the worst that could happen is that these tests would miss a lifetime bug. While that would be unfortunate, it's not significantly worse than the status quo which is no checks at all.

Of course, I'd be happy to rewrite this if there's a way to achieve the same or similar coverage without relying on undefined behavior. Unfortunately, I can't think of one -- if you have any ideas, I'm happy to discuss.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130330