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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Aha, nice, thanks for adding a description, it is a very good thing to do. Like, every commit should be obvious. Even though i know what's going on, nobody else does, so it's good to explain that this is for modeling certain weird LLVM-specific APIs that intentionally always return true.

I guess this checker should eventually be merged with `StdCLibraryFunctionsChecker` and such. I wonder if it's trivial to convert it to use `CallDescription`s and then extend it trivially to C++ functions. See also D54149 <https://reviews.llvm.org/D54149>. This is one more reason why i don't want to reinvent APINotes: i'm responsible for one more re-inventing. I'll see if i can generalize upon all of these.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:31-37
+namespace {
+struct CallTy {
+  bool TruthValue;
+  StringRef Name;
+  Optional<StringRef> Class;
+};
+}
----------------
Let's use `CallDescriptionMap<bool>` (it hasn't landed yet but it's almost there, D62441).

It should save quite some code; feel free to introduce a convenient constructor if it turns out to be hard to produce an initializer list.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:81
+  // Create a note.
+  const NoteTag *CallTag = C.getNoteTag([Call](BugReport &BR) -> std::string {
+    SmallString<128> Msg;
----------------
Please make the note prunable, so that it didn't bring in the nested stack frame if it appears in a nested stack frame (cf. D61817) - you might need to add a new flag to `CheckerContext::getNoteTag()` to make this happen.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:89
+
+    Out << Call->Name << "' always return "
+        << (Call->TruthValue ? "true" : "false");
----------------
"returns"


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:98
+  SVal RetV = CE.getReturnValue();
+  Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>();
+
----------------
`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.


================
Comment at: clang/test/Analysis/return-value-guaranteed.cpp:20
+struct Foo { int Field; };
+bool error();
+bool problem();
----------------
`core.builtin.NoReturnFunctions` reacts on this function. You can easily see it in the explodedgraph dump, as the last sink node in `test_calls::parseFoo` is tagged with that checker. You'll have to pick a different name for testing purposes.


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

https://reviews.llvm.org/D63915