[PATCH] D66486: [LifetimeAnalysis] Detect more cases when the address of a local variable escapes

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/lib/Sema/SemaInit.cpp:6775
+    if (!pathOnlyInitializesGslPointer(Path))
+      Init = const_cast<Expr *>(Init->skipRValueSubobjectAdjustments());
 
----------------
xazax.hun wrote:
> gribozavr wrote:
> > I'm afraid this change could disable some other analysis, which would hide other lifetime issues. For example, while 'ptr' can't itself dangle, if 'Temp().ptr' is bound to a local reference, it might be subject to complex lifetime extension rules, which this warning originally tried to check for.
> I understand your concern, and I thin this code path is very tricky as it is doing multiple things at once: lifetime extension and emitting warnings. There are two cases when `pathOnlyInitializesGslPointer(Path)` returns true:
> * We are initializing a gsl::Pointer, so no references involved, we should not do lifetime extension.
> * We are visiting the initialization of a reference (like walking a def-use chain) that was used to 
> 
> In the second case, the reference initialization was already visited earlier (when we first saw the initialization and did not follow any use of the reference). In this earlier instance, since the reference is not a gsl::Pointer, the call to `pathOnlyInitializesGslPointer` is evaluated to false, and we did the lifetime extension. 
> 
> To observe this behavior you can dump the AST for the following code:
> ```
> namespace std {
> template<typename T>
> struct reference_wrapper {
>   reference_wrapper(T &&);
> };
> 
> template<typename T>
> reference_wrapper<T> ref(T& t) noexcept;
> }
> 
> std::reference_wrapper<const int> treatForwardingRefAsLifetimeConst() {
>   const int &b = int(6);
>   return std::ref(b); 
> }
> ```
> 
> The result for me is:
> ```
> `-FunctionDecl 0xfa5828 <line:34:1, line:37:1> line:34:35 treatForwardingRefAsLifetimeConst 'std::reference_wrapper<const int> ()'
>   `-CompoundStmt 0xfa7848 <col:71, line:37:1>
>     |-DeclStmt 0xfa5e58 <line:35:3, col:24>
>     | `-VarDecl 0xfa5cf0 <col:3, col:23> col:14 used b 'const int &' cinit
>     |   `-ExprWithCleanups 0xfa5de8 <col:18, col:23> 'const int' lvalue
>     |     `-MaterializeTemporaryExpr 0xfa5db8 <col:18, col:23> 'const int' lvalue extended by Var 0xfa5cf0 'b' 'const int &'
>     |       `-CXXFunctionalCastExpr 0xfa5d90 <col:18, col:23> 'int' functional cast to int <NoOp>
>     |         `-IntegerLiteral 0xfa5d70 <col:22> 'int' 6
>     `-ReturnStmt 0xfa7838 <line:36:3, col:20>
>       `-ExprWithCleanups 0xfa7820 <col:10, col:20> 'std::reference_wrapper<const int>':'std::reference_wrapper<const int>'
>         `-CXXConstructExpr 0xfa77f0 <col:10, col:20> 'std::reference_wrapper<const int>':'std::reference_wrapper<const int>' 'void (std::reference_wrapper<const int> &&) noexcept' elidable
>           `-MaterializeTemporaryExpr 0xfa7798 <col:10, col:20> 'reference_wrapper<const int>':'std::reference_wrapper<const int>' xvalue
>             `-CallExpr 0xfa7300 <col:10, col:20> 'reference_wrapper<const int>':'std::reference_wrapper<const int>'
>               |-ImplicitCastExpr 0xfa72e8 <col:10, col:15> 'reference_wrapper<const int> (*)(const int &) noexcept' <FunctionToPointerDecay>
>               | `-DeclRefExpr 0xfa7250 <col:10, col:15> 'reference_wrapper<const int> (const int &) noexcept' lvalue Function 0xfa7150 'ref' 'reference_wrapper<const int> (const int &) noexcept' (FunctionTemplate 0xfa5500 'ref')
>               `-DeclRefExpr 0xfa5ed8 <col:19> 'const int' lvalue Var 0xfa5cf0 'b' 'const int &'
> ```
> 
> Please let me know if you had a different example in mind.
> 
> (Also, this unfortunately is a false negative example for now, as we tend to start with the less risky warnings :))
Thank you for the explanation. I don't feel I understand this code enough to accept such a subtle explanation for a change with potentially far-reaching consequences.

The "path" abstraction that this analysis is designed to catch problems with lifetime extension, and so far it seemed like we could tack our analysis onto it, but now it looks like the wrong abstraction for this purpose.

If you can find another reviewer who is more comfortable with this code, and they accept the patch, I have no objections though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66486