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

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

That's exactly my experience with compiler warnings and tools like Asan -- all of them are essentially "best effort" and aren't reliable ("reliable" in the sense "guaranteed to catch 100% of issues"). In fact, when I did the first patch to fix the Chromium issue, Clang could see the dangling temporary in the original version of `__iter_move` (that returned a dangling `value_type`) but not in the version from the patch (that returned a dangling `reference`), even though both were equally UB and essentially the same issue. You encountered a very similar problem where GCC sees a lifetime issue while Clang doesn't. I'm sure there exist counterexamples where Clang would catch something that GCC cannot spot. The point is, these warnings aren't meant to validate that the code is correct -- while their presence almost always indicates a problem, their absence doesn't guarantee there is no problem. Asan similarly cannot catch all issues. To be clear, all these tools are very helpful, but should be used for their intended purpose and not to verify that code is correct.

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

The assertion isn't triggered because the program segfaults on the line that calls `iter_move`. Unfortunately, Godbolt output doesn't make it very clear, but `139` isn't the value of `i`, it's the return code of a program upon receiving `sigsegv` (`11`, which is the code of the signal, + `128`).

>From the perspective of the standard, the Godbolt program might be equivalently undefined compared to this patch, but in practice there is a very important difference -- the program dereferences `this` after the destructor has been run, while this patch doesn't.

To be clear, I would love to do it another way that doesn't require this sort of "this is technically undefined but works in practice" analysis. Unfortunately, I don't really see it. `constexpr` would be the perfect solution if it were not for the coverage problems. I think that tooling works best as complimentary, not the exclusive means of detecting this sort of issue.

There are some possible mitigations we could do as well. We can make sure this test runs without optimizations, we may restrict it to a certain compiler version if need be, and we could add a `fail.cpp` test to make sure it actually fails in practice (when encountering memory problems). I think this is overkill, personally, but wouldn't object if you feel strongly about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130330