[analyzer] Disable constructor inlining when lifetime extension through fields occurs.

[NoQ]

Summary

As i mentioned in D43497, automatic destructors are missing in the CFG in situations like

const int &x = C().x;

For now i'd rather disable construction inlining, because inlining constructors while doing nothing on destructors is very bad.

[dcoughlin]

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

70

Would you be willing to add a tiny code example to the comment? I.e., const int &x = C().x

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

177

I *think* this is safe. But it seems like it would be more direct to use skipRValueSubobjectAdjustments() and check for sub-object adjustments since ultimately that is what you care about, right?

[NoQ]

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

177

skipRValueSubobjectAdjustments()

This function doesn't do anything anymore (since rC288563) (in most cases, at least). We now have lvalue sub-objects adjustments that are outside of MaterializeTemporaryExpr, and i'm trying to see if there are any by comparing the variable type with the temporary type. If there are still any rvalue sub-object adjustments present in the AST, then i'd have to skip them, as an additional step.

[NoQ]

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

70

Fixed in the commit.

[chh]

This change caused an assertion failure in ExprEngineCXX.cpp:
https://bugs.llvm.org/show_bug.cgi?id=37166

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

struct EvalCallOptions {
     /// This call is a constructor or a destructor for which we do not currently
     /// compute the this-region correctly.
     bool IsCtorOrDtorWithImproperlyModeledTargetRegion = false;
     /// This call is a constructor or a destructor for a single element within
     /// an array, a part of array construction or destruction.
     bool IsArrayCtorOrDtor = false;
     /// This call is a constructor or a destructor of a temporary value.
     bool IsTemporaryCtorOrDtor = false;
+    /// This call is a constructor for a temporary that is lifetime-extended
+    /// by binding a smaller object within it to a reference, for example
+    /// 'const int &x = C().x;'.
+    bool IsTemporaryLifetimeExtendedViaSubobject = false;
 
     EvalCallOptions() {}
   };
 
 private:
   AnalysisManager &AMgr;
   
   AnalysisDeclContextManager &AnalysisDeclContexts;

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::NewAllocatedObjectKind: {
                 MR, NE->getType()->getPointeeType());
           }
           return MR;
         }
       }
       break;
     }
     case ConstructionContext::TemporaryObjectKind: {
+      const auto *TOCC = cast<TemporaryObjectConstructionContext>(CC);
+      // See if we're lifetime-extended via our field. If so, take a note.
+      // Because automatic destructors aren't quite working in this case.
+      if (const auto *MTE = TOCC->getMaterializedTemporaryExpr()) {
+        if (const ValueDecl *VD = MTE->getExtendingDecl()) {
+          assert(VD->getType()->isReferenceType());
+          if (VD->getType()->getPointeeType().getCanonicalType() !=
+              MTE->GetTemporaryExpr()->getType().getCanonicalType()) {
+            CallOpts.IsTemporaryLifetimeExtendedViaSubobject = true;
+          }
+        }
+      }
       // TODO: Support temporaries lifetime-extended via static references.
       // They'd need a getCXXStaticTempObjectRegion().
       CallOpts.IsTemporaryCtorOrDtor = true;
       return MRMgr.getCXXTempObjectRegion(CE, LCtx);
     }
     case ConstructionContext::ReturnedValueKind: {
       // TODO: We should construct into a CXXBindTemporaryExpr or a
       // MaterializeTemporaryExpr around the call-expression on the previous

lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp

if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete) {
           !Opts.includeTemporaryDtorsInCFG())
         return CIP_DisallowedOnce;
 
       // If we did not find the correct this-region, it would be pointless
       // to inline the constructor. Instead we will simply invalidate
       // the fake temporary target.
       if (CallOpts.IsCtorOrDtorWithImproperlyModeledTargetRegion)
         return CIP_DisallowedOnce;
+
+      // If the temporary is lifetime-extended by binding a smaller object
+      // within it to a reference, automatic destructors don't work properly.
+      if (CallOpts.IsTemporaryLifetimeExtendedViaSubobject)
+        return CIP_DisallowedOnce;
     }
 
     break;
   }
   case CE_CXXDestructor: {
     if (!Opts.mayInlineCXXMemberFunction(CIMK_Destructors))
       return CIP_DisallowedAlways;
 

test/Analysis/lifetime-extension.cpp

struct A {
   ~A() {}
 };
 
 void f() {
   const int &x = A().i; // no-crash
   const int &y = A().j[1]; // no-crash
   const int &z = (A().j[1], A().j[0]); // no-crash
 
-  clang_analyzer_eval(x == 1);
-  clang_analyzer_eval(y == 3);
-  clang_analyzer_eval(z == 2);
-#ifdef TEMPORARIES
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
- // expected-warning@-4{{TRUE}}
-#else
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
- // expected-warning@-8{{UNKNOWN}}
-#endif
+  // FIXME: All of these should be TRUE, but constructors aren't inlined.
+  clang_analyzer_eval(x == 1); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(y == 3); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(z == 2); // expected-warning{{UNKNOWN}}
 }
 } // end namespace pr19539_crash_on_destroying_an_integer
 
 namespace maintain_original_object_address_on_lifetime_extension {
 class C {
   C **after, **before;
 
 public:

 }
 
 void f5() {
   C *after, *before;
   {
     const bool &x = C(true, &after, &before).x; // no-crash
   }
   // FIXME: Should be TRUE. Should not warn about garbage value.
-  clang_analyzer_eval(after == before);
-#ifdef TEMPORARIES
-  // expected-warning@-2{{The left operand of '==' is a garbage value}}
-#else
-  // expected-warning@-4{{UNKNOWN}}
-#endif
+  clang_analyzer_eval(after == before); // expected-warning{{UNKNOWN}}
 }
 } // end namespace maintain_original_object_address_on_lifetime_extension
 
 namespace maintain_original_object_address_on_move {
 class C {
   int *x;
 
 public: