[PATCH] D63915: [analyzer] ReturnValueChecker: Model the guaranteed boolean return value of function calls

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent &Call) const {
+  Optional<T> lookup(const CallEvent &Call) const {
     // Slow path: linear lookup.
----------------
I hope `T` never gets too expensive to copy. The ideal return value here is `Optional<const T &>` but i suspect that `llvm::Optional`s don't support this (while C++17 `std::optional`s do). Could you double-check my vague memories here?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1126
 
-    return nullptr;
+    return {};
   }
----------------
People usually use `None` in such cases; i guess that's because it's slightly more explicit.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:98
+  SVal RetV = CE.getReturnValue();
+  Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>();
+
----------------
Charusso wrote:
> NoQ wrote:
> > `castAs` if you're sure.
> > 
> > Generally it's either evaluated conservatively (so the return value is a conjured symbol) or inlined (and in this case returning an undefined value would result in a fatal warning), so return values shouldn't ever be undefined.
> > 
> > The only way to obtain an undefined return value is by binding it in `evalCall()`, but even then, i'd rather issue a warning immediately.
> Sometimes it failed with live tests in the wild.
I'm mildly interested, can you share a repro if you happen to still have one?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:14
+
+#include "clang/Basic/IdentifierTable.h"
+#include "clang/Driver/DriverDiagnostic.h"
----------------
Do we still need this include?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:39
+      // 'Error()'
+      {{{"ARMAsmParser", "Error"}}, true},
+      {{{"HexagonAsmParser", "Error"}}, true},
----------------
We can play even more nicely here by requiring namespace `llvm` as well (just prepend one more item to the list).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:52
+      {{{"TGParser", "TokError"}}, true},
+      // FIXME: 'error()': NoReturnFunctionChecker overlap.
+      {{{"MIParser", "error"}}, true},
----------------
Nope. `NoReturnFunctionChecker` only reacts on C functions.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82
+
+        Out << '\'' << Name << "' always returns "
+            << (*Value ? "true" : "false");
----------------
Let's mention the class name as well! Maybe even the fully qualified namespace.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:89-91
+  // Set the concrete return value.
+  State = State->assume(*RetDV, *Value);
+  C.addTransition(State, CallTag);
----------------
I'd like to see what happens when `State->assume()` returns null. This may happen when the function is inlined and proved to return an unexpected return value. Say, if we believe that the function is always true but the code changes and it suddenly starts returning false and we inline it. Let's add such test case and make sure we behave sanely. Not sure what should the sane behavior be; the safe thing to do would be to generate sink, but we may also try to leave a note telling that something weird has happened ("note: MCParser::Error() SUDDENLY returns false" and mark the inlined stack frame as interesting).


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

https://reviews.llvm.org/D63915