[CFG] [analyzer] Fix a crash on finding construction context for an lvalue/xvalue call expression.

[NoQ]

Summary

The type of the call expression and the return value type of the function are not necessarily the same thing. In the attached example, the function is declared to return const C &, but the actual call-expression has type const C and is also an lvalue. Which is, well, kinda the same thing, but needs a different sort of boilerplate.

Previously we tried to find construction context for such call-expression and became surprised that this is not a temporary object context. Fix the crash by making sure that the call-expression is not an lvalue before trying to attach a construction context to it.

[NoQ]

Same goes for rvalue references. Hmm, we have a fancy helper method for that exact purpose. But it's not super easy to use, had to sacrifice a sanity check assertion to use it.

[NoQ]

Fix typo in the comments.

[NoQ]

Add the tests to a CFG-oriented test file as well, because it doesn't have much to do with the analyzer.

FIXME: temp-obj-dtors-cfg-output.cpp is a mess. It'd be great to interleave code and expected output, like other CFG tests do, to make it readable.

include/clang/Analysis/CFG.h

 /// TemporaryObjectConstructionContext at the call site (here). In C such
 /// tracking is not necessary because no additional effort is required for
 /// destroying the object or modeling copy elision. Like CFGConstructor, this is
 /// for now only used by the analyzer's CFG.
 class CFGCXXRecordTypedCall : public CFGStmt {
 public:
   /// Returns true when call expression \p CE needs to be represented
   /// by CFGCXXRecordTypedCall, as opposed to a regular CFGStmt.
-  static bool isCXXRecordTypedCall(CallExpr *CE) {
-    return CE->getType().getCanonicalType()->getAsCXXRecordDecl();
+  static bool isCXXRecordTypedCall(CallExpr *CE, const ASTContext &ACtx) {
+    return CE->getCallReturnType(ACtx).getCanonicalType()->getAsCXXRecordDecl();
   }
 
   explicit CFGCXXRecordTypedCall(CallExpr *CE,
                                  const TemporaryObjectConstructionContext *C)
       : CFGStmt(CE, CXXRecordTypedCall) {
-    assert(isCXXRecordTypedCall(CE));
+    // FIXME: This is not protected against squeezing a non-record-typed-call
+    // into the constructor. An assertion would require passing an ASTContext
+    // which would mean paying for something we don't use.
     assert(C);
     Data2.setPointer(const_cast<TemporaryObjectConstructionContext *>(C));
   }
 
   const TemporaryObjectConstructionContext *getConstructionContext() const {
     return static_cast<TemporaryObjectConstructionContext *>(
         Data2.getPointer());
   }

lib/Analysis/CFG.cpp

void appendConstructor(CFGBlock *B, CXXConstructExpr *CE) {
     }
 
     // No valid construction context found. Fall back to statement.
     B->appendStmt(CE, cfg->getBumpVectorContext());
   }
 
   void appendCall(CFGBlock *B, CallExpr *CE) {
     if (BuildOpts.AddRichCXXConstructors) {
-      if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE)) {
+      if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE, *Context)) {
         if (const ConstructionContextLayer *Layer =
                 ConstructionContextMap.lookup(CE)) {
           const ConstructionContext *CC =
               ConstructionContext::createFromLayers(cfg->getBumpVectorContext(),
                                                     Layer);
           B->appendCXXRecordTypedCall(
               CE, cast<TemporaryObjectConstructionContext>(CC),
               cfg->getBumpVectorContext());
void CFGBuilder::findConstructionContexts(
   // FIXME: This, like the main visit, doesn't support CUDAKernelCallExpr.
   // FIXME: An isa<> would look much better but this whole switch is a
   // workaround for an internal compiler error in MSVC 2015 (see r326021).
   case Stmt::CallExprClass:
   case Stmt::CXXMemberCallExprClass:
   case Stmt::CXXOperatorCallExprClass:
   case Stmt::UserDefinedLiteralClass: {
     auto *CE = cast<CallExpr>(Child);
-    if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE))
+    if (CFGCXXRecordTypedCall::isCXXRecordTypedCall(CE, *Context))
       consumeConstructionContext(Layer, CE);
     break;
   }
   case Stmt::ExprWithCleanupsClass: {
     auto *Cleanups = cast<ExprWithCleanups>(Child);
     findConstructionContexts(Layer, Cleanups->getSubExpr());
     break;
   }

test/Analysis/temp-obj-dtors-cfg-output.cpp

int testConsistencyNestedNormalReturn(bool value) {
   if (value) {
     if (!value || value || check(NoReturn())) {
       return 1;
     }
   }
   return 0;
 }
 
+namespace pass_references_through {
+class C {
+public:
+  ~C() {}
+};
+
+const C &foo1();
+C &&foo2();
+
+// In these examples the foo() expression has record type, not reference type.
+// Don't try to figure out how to perform construction of the record here.
+const C &bar1() { return foo1(); } // no-crash
+C &&bar2() { return foo2(); } // no-crash
+} // end namespace pass_references_through
+
 // CHECK:   [B1 (ENTRY)]
 // CHECK:     Succs (1): B0
 // CHECK:   [B0 (EXIT)]
 // CHECK:     Preds (1): B1
 // CHECK:   [B1 (ENTRY)]
 // CHECK:     Succs (1): B0
 // CHECK:   [B0 (EXIT)]
 // CHECK:     Preds (1): B1

 // CHECK:   [B9]
 // CHECK:     1: value
 // CHECK:     2: [B9.1] (ImplicitCastExpr, LValueToRValue, _Bool)
 // CHECK:     T: if [B9.2]
 // CHECK:     Preds (1): B10
 // CHECK:     Succs (2): B8 B1
 // CHECK:   [B0 (EXIT)]
 // CHECK:     Preds (3): B1 B2 B4
+// CHECK:   [B1 (ENTRY)]
+// CHECK:     Succs (1): B0
+// CHECK:   [B0 (EXIT)]
+// CHECK:     Preds (1): B1
+// CHECK:   [B2 (ENTRY)]
+// CHECK:     Succs (1): B1
+// CHECK:   [B1]
+// CHECK:     1: foo1
+// CHECK:     2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, const class pass_references_through::C &(*)(void))
+// CHECK:     3: [B1.2]()
+// CHECK:     4: return [B1.3];
+// CHECK:     Preds (1): B2
+// CHECK:     Succs (1): B0
+// CHECK:   [B0 (EXIT)]
+// CHECK:     Preds (1): B1
+// CHECK:   [B2 (ENTRY)]
+// CHECK:     Succs (1): B1
+// CHECK:   [B1]
+// CHECK:     1: foo2
+// CHECK:     2: [B1.1] (ImplicitCastExpr, FunctionToPointerDecay, class pass_references_through::C &&(*)(void))
+// CHECK:     3: [B1.2]()
+// CHECK:     4: return [B1.3];
+// CHECK:     Preds (1): B2
+// CHECK:     Succs (1): B0
+// CHECK:   [B0 (EXIT)]
+// CHECK:     Preds (1): B1

test/Analysis/temporaries.cpp

 #ifdef TEMPORARY_DTORS
   // expected-warning@-2{{TRUE}}
 #else
   // expected-warning@-4{{UNKNOWN}}
 #endif
 }
 } // end namespace implicit_constructor_conversion
 
+namespace pass_references_through {
+class C {
+public:
+  ~C() {}
+};
+
+const C &foo1();
+C &&foo2();
 
+// In these examples the foo() expression has record type, not reference type.
+// Don't try to figure out how to perform construction of the record here.
+const C &bar1() { return foo1(); } // no-crash
+C &&bar2() { return foo2(); } // no-crash
+} // end namespace pass_references_through