[analyzer] Fix autodetection of getSVal()'s type argument.

[NoQ]

Summary

In ProgramState::getSVal(Loc, QualType), the QualType parameter, which represents the type of the expected value, is optional. If it is not supplied, it is auto-detected by looking at the memory region in Loc. For typed-value regions such autodetection is trivial. For other kinds of regions (most importantly, for symbolic regions) it apparently never worked correctly: it detected the location type (pointer type), not the value type in this location (pointee type).

Our tests contain numerous cases when such autodetection was working incorrectly, but for existing tests it never mattered. There are three notable places where the type was regularly auto-detected incorrectly:

1. ExprEngine::performTrivialCopy(). Trivial copy from symbolic references never worked - test case attached.
2. CallAndMessageChecker::uninitRefOrPointer(). Here the code only cares if the value is Undefined or not, so autodetected type didn't matter.
3. GTestChecker::modelAssertionResultBoolConstructor(). This is how the issue was found in https://bugs.llvm.org/show_bug.cgi?id=34305 - test case attached.

I added a few more sanity checks - we'd now also fail with an assertion if we are unable to autodetect the pointer type, warning the author of the checker to pass the type explicitly.

It is sometimes indeed handy to put a void-pointer-typed Loc into getSVal(Loc), as demonstrated in the exercise-ps.c's f2() test through CallAndMessageChecker (case '2.' above). I handled this on the API side in order to simplify checker API, implicitly turning void into char, though i don't mind modifying CallAndMessageChecker to pass CharTy explicitly instead.

[NoQ]

Add a forgotten comment.

[NoQ]

Add @alexfh's small reproducer test case. It was so small i never noticed it until now!

[alexfh]

Thank you for the fix! It seems to be the largest source of SA crashes on our code at the moment.

[dcoughlin]

This is such a nasty bug! It is great to see a fix. I have two comments inline, one of which is just a nit.

lib/StaticAnalyzer/Core/RegionStore.cpp

1404

Nit: It seems a bit odd to read the first byte here since (unless I'm misunderstanding) this would never be triggered by actual C semantics, only by a checker. Did you consider just returning UnknownVal() in this case?

1408

I think you missed handling the AllocaRegion case from the old version in your new version. This means the assert will fire on the following when core.alpha is enabled:

void foo(void *dest) {
  void *src = __builtin_alloca(5);
  memcpy(dest, src, 1);
}

[NoQ]

Yeah, nice catch. So we need to either always tell the checkers to specify their CharTy when they are dealing with void pointers, or to do our substitution consistently, not only for SymbolicRegion but also for AllocaRegion (did that in this diff).

[dcoughlin]

Looks great to me.

I do wonder if long-term we should consider removing the auto-deduction when loading from the store. On the one hand it is really nice to avoid having to specify that at the call to getSVal(). On the other hand, this can lead to some really pathological weirdness and a bunch of corner-case code. For loads that result from actual program semantics the type of the loaded-from storage (from the loader's perspective) should always be easily available at the load site. There would still be cases where a checker might want to inspect what the store thinks is in memory, but I think that is a different function and in my opinion deserves a separate API.

[NoQ]

Whoops forgot to submit inline comments.

lib/StaticAnalyzer/Core/RegionStore.cpp

1404

UnknownVal seems to be quite similar to assertion failure: in both cases we'd have to force the checker to specify CharTy explicitly. But assertions are better because the author of the checker would instantly know that he needs to specify the type, instead of never noticing the problem, or spending a lot of time in the debugger trying to understand why he gets an UnknownVal.

1408

Nice one!

Always been that way though. In the old code, if AllocaRegion is supplied and no type is explicitly specified, cast<SymbolicRegion>(MR) would fail. Also ElementRegion is created in both old and new code if the type was specified.

[dcoughlin]

lib/StaticAnalyzer/Core/RegionStore.cpp

1404

I think having an assertion with a helpful message indicating how the checker author could fix the problem would be great! But you're not triggering an assertion failure here since you're changing T to be CharTy. Instead, you're papering over the problem by making up a fake value that doesn't correspond to the program semantics. If you're going to paper over the the problem and return a value, I think it is far preferable to use UnknownVal. This would signal "we don't know what is going here" rather than just making up a value that is likely wrong.

[NoQ]

lib/StaticAnalyzer/Core/RegionStore.cpp

1404

If you're going to paper over the the problem and return a value, I think it is far preferable to use UnknownVal.

If we return an UnknownVal, the user would never figure out what's wrong by looking at the value, i.e. what limitation of the analyzer he's stepping on (in fact he isn't, he's just using the APIs incorrectly, while we know very well what's going on). Additionally, returning the first byte makes API simpler in cases the type actually doesn't matter (which is why it's not provided), unlike returning UnknownVal. This makes me think that returning UnknownVal is worse than both returning first byte and stepping on assertion. However, yeah, if we try to model an API that manipulates objects of well-defined types through void pointers, returning bytes might cause issues when the user doesn't realize he needs to specify his types, especially when RegionStore would often ignore the type and only take the offset, unless it has no bindings and needs to return region value or derived symbol (so the user may easily fail to test this case).

I guess i'd follow up with a patch to remove the defensive behavior and bring back the assertion and modify checkers to provide types when necessary, and later see if it's worth it to remove auto-detection completely.

lib/StaticAnalyzer/Core/RegionStore.cpp

SVal RegionStoreManager::getBinding(RegionBindingsConstRef B, Loc L, QualType T) {
   }
 
   const MemRegion *MR = L.castAs<loc::MemRegionVal>().getRegion();
 
   if (isa<BlockDataRegion>(MR)) {
     return UnknownVal();
   }
 
-  if (isa<AllocaRegion>(MR) ||
-      isa<SymbolicRegion>(MR) ||
-      isa<CodeTextRegion>(MR)) {
+  if (!isa<TypedValueRegion>(MR)) {
     if (T.isNull()) {
-      if (const TypedRegion *TR = dyn_cast<TypedRegion>(MR))
-        T = TR->getLocationType();
-      else {
-        const SymbolicRegion *SR = cast<SymbolicRegion>(MR);
-        T = SR->getSymbol()->getType();
+      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();
+        if (T->isVoidType()) {
+          // When trying to dereference a void pointer, read the first byte.
+          T = Ctx.CharTy;
+        }
       }
     }
+    assert(!T.isNull() && "Unable to auto-detect binding type!");
+    assert(!T->isVoidType() && "Attempted to retrieve a void value!");
     MR = GetElementZeroRegion(cast<SubRegion>(MR), T);
   }
 
   // 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();
 

test/Analysis/ctor.mm

public:
         y = Other.y;
         return *this;
       }
     };
 
     Inner p;
   };
 
-  void testPOD() {
+  void testPOD(const POD &pp) {
     POD p;
     p.x = 1;
     POD p2 = p; // no-warning
     clang_analyzer_eval(p2.x == 1); // expected-warning{{TRUE}}
     POD p3 = move(p); // no-warning
     clang_analyzer_eval(p3.x == 1); // expected-warning{{TRUE}}
 
     // Use rvalues as well.
     clang_analyzer_eval(POD(p3).x == 1); // expected-warning{{TRUE}}
 
+    // Copy from symbolic references correctly.
+    POD p4 = pp;
+    // Make sure that p4.x contains a symbol after copy.
+    if (p4.x > 0)
+      clang_analyzer_eval(p4.x > 0); // expected-warning{{TRUE}}
+    // FIXME: Element region gets in the way, so these aren't the same symbols
+    // as they should be.
+    clang_analyzer_eval(pp.x == p4.x); // expected-warning{{UNKNOWN}}
+
     PODWrapper w;
     w.p.y = 1;
     PODWrapper w2 = w; // no-warning
     clang_analyzer_eval(w2.p.y == 1); // expected-warning{{TRUE}}
     PODWrapper w3 = move(w); // no-warning
     clang_analyzer_eval(w3.p.y == 1); // expected-warning{{TRUE}}
 
     // Use rvalues as well.

test/Analysis/gtest.cpp

 void testConstrainState(int p) {
   ASSERT_TRUE(p == 7);
 
   clang_analyzer_eval(p == 7); // expected-warning {{TRUE}}
 
   ASSERT_TRUE(false);
   clang_analyzer_warnIfReached(); // no-warning
 }
+
+void testAssertSymbolic(const bool &b) {
+  ASSERT_TRUE(b);
+
+  clang_analyzer_eval(b); // expected-warning{{UNKNOWN}}
+}