[PATCH] D64374: [analyzer] CastValueChecker: Model casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:25-27
+  using CastCheck =
+      std::function<void(const CastValueChecker *, const CallExpr *,
+                         DefinedOrUnknownSVal, CheckerContext &)>;
----------------
Kinda nice, but a simple pointer-to-member would have been sufficient and much more lightweight:

```lang=c++
using CastCheck = void (CastValueChecker::*)(const CallExpr *,
                         DefinedOrUnknownSVal, CheckerContext &);
```

(not sure, i don't fully understand the performance implications of using `std::function`)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:40-43
+      {{{"cast"}}, &CastValueChecker::evalCast},
+      {{{"dyn_cast"}}, &CastValueChecker::evalDynCast},
+      {{{"cast_or_null"}}, &CastValueChecker::evalCastOrNull},
+      {{{"dyn_cast_or_null"}}, &CastValueChecker::evalDynCastOrNull}};
----------------
Please add an argument count check to your `CallDescription`s.

Otherwise accessing argument 0 may result in a crash when we're analyzing code that isn't LLVM but defines a function with the same name which also has no arguments.

That's a fairly common source of crashes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:60-63
+  if (!State->assume(ParamDV, true))
+    return;
+
+  State = State->assume(ParamDV, true);
----------------
The `assume` operation is expensive. Let's avoid doing it twice:

```lang=c++
  State = State->assume(ParamDV, true);
  if (!State)
    return;
```


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:67-68
+
+  std::string CastToName = CE->getType().getAsString();
+  std::string CastFromName = CE->getArg(0)->getType().getAsString();
+
----------------
Let's do `...getType()->getPointeeCXXRecordDecl()->getNameAsString()`. It's shorter and more on point. (might need to check for null decl though).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:75
+
+        Out << "Cast to '" << CastToName << "' (from '" << CastFromName << "')";
+        return Out.str();
----------------
This statement needs a predicate.

I suggest: `Assuming dynamic cast from 'A' to 'B' succeeds`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:78
+      },
+      /*IsPrunable=*/false);
+
----------------
`true`! We don't want a note about every random cast to bring in dozens of nested stack frames.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:92
+  State = State->BindExpr(CE, C.getLocationContext(),
+                          C.getSValBuilder().makeNull(), /*Invalidate=*/false);
+
----------------
Just drop `Invalidate`. Situations where you actually notice the difference are extremely rare. In this case there's nothing to invalidate because there's no existing value for the call-expression.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:100
+
+        Out << "Cast to '" << CastToName << "' results in a null pointer";
+        return Out.str();
----------------
`Assuming dynamic cast from 'A' to 'B' fails`.

Later when we add support for dynamic type tracking, we may as well add a "Knowing..." piece :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:175
+  else*/
+    Param = CE->getArg(0);
+
----------------
Charusso wrote:
> This CXXMemberCallExpr business is necessary? I am not sure as I have not seen it widely used.
It definitely needs different handling because `this` cannot ever be null.

If you haven't seen it widely used, it's probably not necessary; i too rarely see those.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:177
+
+  SVal ParamV = C.getState()->getSVal(Param, C.getLocationContext());
+  auto ParamDV = ParamV.getAs<DefinedOrUnknownSVal>();
----------------
`Call.getArgSVal(0)`.


================
Comment at: clang/test/Analysis/cast-value.cpp:34
+    // expected-note at -4 {{Taking true branch}}
+    (void)(1 / !Baz);
+    // expected-note at -1 {{'Baz' is non-null}}
----------------
`debug.ExprInspection` is your friend!

I'd also call for making the tests more isolated. In this test you're hardcoding a fairly arbitrary and random execution path that the analyzer chose through your function. It's generally interesting what paths do we explore first, but that's not what the patch is about.

Here's an example of a test that i would have written:
```lang=c++
void foo(Shape *S) {
  Circle *C = dyn_cast_or_null<Circle>(S);
  clang_analyzer_numTimesReached(); // expected-warning{{3}}
  if (S && C)
    clang_analyzer_eval(C == S); // expected-warning{{TRUE}}
  if (!S)
    clang_analyzer_eval(!C); // expected-warning{{TRUE}}
  if (S && !C)
    clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
}
```
(you might want to test notes separately)


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64374/new/

https://reviews.llvm.org/D64374