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

[Hui via Phabricator via libcxx-commits]

huixie90 added a comment.

In D130330#3672322 <https://reviews.llvm.org/D130330#3672322>, @var-const wrote:

> 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.

Hi, thanks for the explanation. Undefined behavior is not something very reliable.
My original test has some coverage (would fail on gcc). I am trying to figure out why clang passes the test. See this reproduction example (it is basically my original test plus making `Reference` destructor to set `_i` to be `nullptr`
https://godbolt.org/z/Y3j4GGjxv
It is clear that the behavior of the original `__iter_move` is already wrong according to clang output. If the program behaves correctly, `i` should equal to `5` and the program should `return 5`. But the program return 139 which means something clearly wrong.
However, the assertion `assert(i==5)` isn't triggered because of the Undefined Behaviour. The compiler doesn't even bother `assert` as `i` is just rubbish value.

If we run the test with -fsantize=address, it would catch the issue
https://godbolt.org/z/q7Khc91T7


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130330