[analyzer] Prevent crashing in NonNullParamChecker

[george.karpenkov]

Summary

https://bugs.llvm.org/show_bug.cgi?id=36381
rdar://37543426

Turns out, the type passed for the lambda capture was incorrect.
One more argument to abandon the getSVal overload which does not require the type information.

[a.sidorin]

Hi George. The commit looks fine, but could you please add a test?

[george.karpenkov]

Updated in favor of setting the type inside RegionStore

[george.karpenkov]

Added a test.

[NoQ]

Here's the way i understand what's going on. @xazax.hun, you'd probably like this one because it's about lambdas. Though in fact, of course, it's about RegionStore - lambda support doesn't seem to be at fault.

1. f(i) on line 6 implicitly copies i in order to pass it into f().
2. That implicit copy-constructor of class C accepts i as const C &.
3. i is captured into the lambda by reference.
4. It means that the lambda object contains a reference-type field that references the actual variable i.
5. At least, it should have been containing that. We didn't actually implement that yet.
6. This field has no name, but for the purposes of this discussion let's call it _i.
7. We do try, however, to retrieve the value for _i while modeling DeclRefExpr i on line 6 - that's already implemented.
8. The lambda is copied twice before we invoke it - first time to variable lambda, second time to *blah.
9. performTrivialCopy makes a direct binding when copying the object, even though it doesn't explicitly intend to.
10. I've no idea why the heap symregion matters, but having at least one copy seems important.
11. Anyway, on line 6 _i is a FieldRegion within that HeapSymRegion.
12. When we try to retrieve the value for field _i, we get into getBindingForFieldOrElementCommon().
13. We immediately see that the whole HeapSymRegion has a direct lazy binding.
14. We return this lazy binding as SVal for field _i, and, therefore, for DeclRefExpr i.
15. This is our problem. i is an lvalue expression, so its defined value must not be a NonLoc.
16. But instead we have a lazyCompoundVal for the whole HeapSymRegion.
17. Once we specify our reference type explicitly, we also have to go through CastRetrievedVal() before returning from getBinding().
18. CastRetrievedVal() fails to cast our lazyCompoundVal into a reference, so it returns UnknownVal.
19. UnknownVal is not a NonLoc, which is acceptable.

The shocking part of this investigation is that for now even if region R is a typed-value region (FieldRegion in our case), specifying the optional type argument T in getSVal(R, T) is mandatory, otherwise the behavior may be completely incorrect. The current version of the patch tries to fix that. The underlying problem, however, remains: we should not be blindly returning a random binding on step 14, even if it's direct. And it is an open question for me whether performTrivialCopy should make a direct binding. Finally, i don't understand why everything else works and why this problem requires a super confusing reproducer with lambdas and heap allocations.

cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp

if (T.isNull()) {
       if (const TypedRegion *TR = dyn_cast<TypedRegion>(MR))
         T = TR->getLocationType()->getPointeeType();
       else if (const SymbolicRegion *SR = dyn_cast<SymbolicRegion>(MR))
         T = SR->getSymbol()->getType()->getPointeeType();
     }
     assert(!T.isNull() && "Unable to auto-detect binding type!");
     assert(!T->isVoidType() && "Attempting to dereference a void pointer!");
     MR = GetElementZeroRegion(cast<SubRegion>(MR), T);
+  } else {
+    T = cast<TypedValueRegion>(MR)->getValueType();
   }
 
   // FIXME: Perhaps this method should just take a 'const MemRegion*' argument
   //  instead of 'Loc', and have the other Loc cases handled at a higher level.
   const TypedValueRegion *R = cast<TypedValueRegion>(MR);
   QualType RTy = R->getValueType();
 
   // FIXME: we do not yet model the parts of a complex type, so treat the

cfe/trunk/test/Analysis/nonnullparamchecker-crash.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+class C {};
+
+// expected-no-diagnostics
+void f(C i) {
+  auto lambda = [&] { f(i); };
+  typedef decltype(lambda) T;
+  T* blah = new T(lambda);
+  (*blah)();
+  delete blah;
+}