[analyzer] Fix a crash in RVO from within blocks.

[NoQ]

Summary

While blocks are an Apple extension to C, i'm adding everybody because my mistake that i'm fixing here may hurt us more when we add more kinds of location contexts, so we shouldn't be making it anywhere.

In the attached test case, getA() returns an object by value, and therefore requires a construction context. Its construction context is that of a value that's immediately* returned (by value, without any sort of conversion), so copy elision (namely, RVO) applies. This means that we unwind the stack of LocationContexts in order to see where is the current callee called from. This way we obtain the construction context for the call site, and from that we can figure out what object is constructed. In this case it's going to be the first-and-only argument of use(). This is all good.

In this case RVO is applied to a return value of an anonymous block that's declared only to be immediately called and discarded. The Static Analyzer models block calls by putting two location contexts on the stack: a BlockInvocationContext followed by the actual StackFrameContext. I don't really know why it does that :) but that's not important, because if we introduce more kinds of location contexts, it'll look similarly anyway.

Therefore the correct idiom for obtaining the parent stack frame is to first obtain the parent, and then don't forget to obtain its stack frame, which is not necessarily the parent itself. This is the mistake that i made in my RVO code that i'm fixing here.

The code was crashing because it was looking up the call site in a CFGStmtMap for the wrong CFG (obtained from a wrong location context). This was happening in CallEvent::getCalleeStackFrame().

[Charusso]

I think the entire LocationContext stuff is a huge design issue, and you used its functionality without any hack. If you would rename the getStackFrame to getNextStackFrame or something, it is clear it is not a getter and traversing every frame until the top-frame.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

203 ↗

(On Diff #198121)

The smallest the scope between the definition and its usage, the better the code. Could you put the new code immediately before the return statement?

[NoQ]

No, this is all wrong. We shouldn't unwrap to the parent stack frame here. We should end up exactly in the location context in which the call has happened. But we should still unwrap the block invocation context, as an exception, because it's just a weird location context that goes before the stack frame but still has its corresponding AnalysisDeclContext tied to the *callee* stack frame.

These block invocation contexts are used, like, once, to help modeling captures. I should probably try to get rid of them entirely.

[NoQ]

Remove unused declaration from the test.

[dcoughlin]

Looks good to me.

lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

case ConstructionContext::CXX17ElidedCopyReturnedValueKind: {
         auto RTC = (*SFC->getCallSiteBlock())[SFC->getIndex()]
                        .getAs<CFGCXXRecordTypedCall>();
         if (!RTC) {
           // We were unable to find the correct construction context for the
           // call in the parent stack frame. This is equivalent to not being
           // able to find construction context at all.
           break;
         }
+        if (isa<BlockInvocationContext>(CallerLCtx)) {
+          // Unwrap block invocation contexts. They're mostly part of
+          // the current stack frame.
+          CallerLCtx = CallerLCtx->getParent();
+          assert(!isa<BlockInvocationContext>(CallerLCtx));
+        }
         return prepareForObjectConstruction(
             cast<Expr>(SFC->getCallSite()), State, CallerLCtx,
             RTC->getConstructionContext(), CallOpts);
       } else {
         // We are on the top frame of the analysis. We do not know where is the
         // object returned to. Conjure a symbolic region for the return value.
         // TODO: We probably need a new MemRegion kind to represent the storage
         // of that SymbolicRegion, so that we cound produce a fancy symbol

test/Analysis/copy-elision.mm

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -fblocks -verify %s
+
+// expected-no-diagnostics
+
+namespace block_rvo_crash {
+struct A {};
+
+A getA();
+void use(A a) {}
+
+void foo() {
+  // This used to crash when finding construction context for getA()
+  // (which is use()'s argument due to RVO).
+  use(^{
+    return getA();  // no-crash
+  }());
+}
+} // namespace block_rvo_crash