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

[Charusso]

Summary

It models the known LLVM methods paired with their class.

[Charusso]

Heavily WIP, see inline.

clang/test/Analysis/return-value-guaranteed.cpp

38

It is blind.

83

It makes no reports.

[Charusso]

• Make it match the class name. (Whoops!)
• Reorder the tuple (swap the return value with the call name).
• Remove the Projects option.
• Remove unnecessary input validation.

[Charusso]

• Fix the note as the class name is optional.

[NoQ]

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 CallDescriptions and then extend it trivially to C++ functions. See also 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.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

32–38

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.

82

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.

90

"returns"

99

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.

clang/test/Analysis/return-value-guaranteed.cpp

21

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.

[NoQ]

clang/test/Analysis/return-value-guaranteed.cpp

21

(yes, this is why your tests aren't working)
(see also D63965)

[Charusso]

• Revert the Calls option. It turned out too difficult to create. (May it useless, as we have tons of similar checkers without any option.)

• Halve the test. We do not want to catch only a call, we have to catch the class as well to prevent error()s like NoReturnFunctionChecker produce.

• Prunable notes added to CheckerContext.

• CallDescriptionMap::lookup() now Optional.

[Charusso]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

99

Sometimes it failed with live tests in the wild.

clang/test/Analysis/return-value-guaranteed.cpp

21

I was not sure why do you mention an unrelated patch, but I realized as the previous graph-rewriter patches, it has my stuff in the summary. That would be the last thing I considered. It is a nice catch, thanks!

[NoQ]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

53

Nope. NoReturnFunctionChecker only reacts on C functions.

99

I'm mildly interested, can you share a repro if you happen to still have one?

[NoQ]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

I hope T never gets too expensive to copy. The ideal return value here is Optional<const T &> but i suspect that llvm::Optionals don't support this (while C++17 std::optionals do). Could you double-check my vague memories here?

1126 ↗

(On Diff #207366)

People usually use None in such cases; i guess that's because it's slightly more explicit.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

15

Do we still need this include?

40

We can play even more nicely here by requiring namespace llvm as well (just prepend one more item to the list).

83

Let's mention the class name as well! Maybe even the fully qualified namespace.

90–92

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).

[NoQ]

clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

36 ↗

(On Diff #207366)

I updated D62441, you might have to rebase >.<

[Charusso]

• Fix.

[Charusso]

In D63915#1563049, @NoQ wrote:

Aha, nice, thanks for adding a description, it is a very good thing to do. Like, every commit should be obvious.

In some of my patches I have not added a description because they are so tiny changes.

I guess this checker should eventually be merged with StdCLibraryFunctionsChecker and such.

Do you have any plans for that?

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

Optional<const T *> is working and used widely, I like that.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

40

Well, I have checked 4 of them, they are in the anonymous namespace, so I will leave it.

83

The class::call part would be tricky, because you need to hook what do you have in the CallDescription. It could be done with the decl-matching part, but then you have to rewrite the CallDescriptionMap interface as lookup(), key(), value(), so you could use the hooked info everywhere. It would require too much overhead for a print.

99

After the patch finishes I will make tests and try to break it again.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

83

Just use CXXMethodDecl::getParent().

[Charusso]

• I do not like Optional<const T *> anymore.
• More simple notes.

[Charusso]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

83

Thanks, I really wanted to have a generic solution.

[NoQ]

This is starting to look great, thanks!

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

97

Let's omit the word "always" here, as we know that there are exceptions from this rule. This may look bad if both Parser::Error() always returns true and Parser::Error() returns false appear in the same report.

121

LLVM coding standard is a fairly specific document: https://llvm.org/docs/CodingStandards.html . It doesn't seem to say anything about parsers.

Let's make this much softer: Parser::Error() returns false and that's it.

Also given that this note always applies to inlined calls, let's move this logic to checkEndFunction(). I.e., we emit the "false" note in checkEndFunction but we emit the "true" note in checkPostCall.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

72

auto is encouraged here.

87

Either cast<> or check for null.

117

This isn't the stack frame i was talking about, but if you move this code to checkEndFunction it will be.

[Charusso]

• Create FunctionExitPoint diagnostics.
• Fix.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

121

Well, we have tons of unspoken standards like: MR, DRE, SE, RD, V.... all makes sense, but I like your idea more.

[Szelethus]

I'd love to chip in later, if you don't mind, but here is just a couple things that caught my mind that I'd like to share before falling asleep ;)

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

This isn't true: the user may decide to only enable non-pathsensitive checkers.

I think the comment should rather state that these whether these checkers are enabled shouldn't be explicitly specified, unless in extreme circumstances (causes a crash in a release build?).

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

228–229 ↗

(On Diff #207668)

Hmm, we use interestingness (markInteresting()) already to determine whether we should prune events or not, maybe we could (in the long term) try to make these mechanisms work in harmony.

In any case, could you please add comments about the new parameter to the class doc? :)

[NoQ]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

228–229 ↗

(On Diff #207668)

Hmm, we use interestingness (markInteresting()) already to determine whether we should prune events or not, maybe we could (in the long term) try to make these mechanisms work in harmony.

These are two fairly different mechanisms to prevent pruning, but both seem useful. The location context is prunable as long as it's not interesting and there are no non-prunable notes emitted within it.

All basic nodes are prunable; all checker notes are non-prunable; some trackExpressionValue notes are non-prunable. This way the really important notes (such as checker notes) never get lost. And it's kinda nice and easy to understand.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

108

Something's wrong with formatting.

110

I'd still like to avoid "should". We are in no position to teach them what should their method return. It's not a matter of convention; it's a matter of correctness. We don't want people to go fix their code to return true when they see a note. We only need to point out that the return value is not what they expect.

I suggest removing this note entirely, i.e., early-return when we see an invariant break in checkPostCall, because we've already emitted a note in checkEndFunction.

146

I think it would be more precise to talk about "expected"/"actual" return value. The actual return value may be either concrete (i.e., nonloc::ConcreteInt) or symbolic (i.e., nonloc::SymbolVal).

Also, there's a relatively famous rule of a thumb for making good comments: "comments shouldn't explain what does the code do, but rather why does it do it". Instead of these comments i'd rather have one large comment at the beginning of checkEndFunction explaining what are we trying to do here.

146–158

I think you can squeeze all this code into one big isInvariantBreak(Call, ReturnV) and re-use it across both methods.

171

Something's wrong with formatting.

[Charusso]

• Fix.
• Refactor.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

Well, I have removed it instead. Makes no sense, you are right.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

146

Great advice, thanks you!

[Charusso]

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

210 ↗

(On Diff #207703)

It makes perfectly no sense here. Is it the mentioned "class doc"?

[Szelethus]

This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

I don't think it's a good idea -- we definitely should eventually be able to list packages with descriptions just like checkers (there actually is a FIXME in CheckerRegistry.cpp for that), but this is the next best thing that we have.

How about this:

// The APIModeling package is for checkers that model APIs and don't perform
// any diagnostics. Checkers within this package are enabled by the core or
// through checker dependencies, so one shouldn't enable/disable them by
// hand (unless they cause a crash, but that will cause dependent checkers to be
// implicitly disabled).

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

Why do we need the optional AND the pointer? How about we just return with a nullptr if we fail to find the call?

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

210 ↗

(On Diff #207703)

You're right. I meant to add it to NoteTags class doc, but whether a NoteTag is prunable isn't a property of NoteTag itself, but rather the infrastructure around it. Oops! I think above const NoteTag *getNoteTag() would be better.

clang/test/Analysis/return-value-guaranteed.cpp

91

Was it? I just tried it out and it doesn't seem to be the case.

[NoQ]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

I don't think any of these are dependencies. Most of the apiModeling checkers are there to suppress infeasible paths (exactly like this one).

I think i'd prefer to leave the comment as-is. We can always update it later.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

Well, that'd be the original code.

I do not like Optional<const T *> anymore.

@Charusso, do you still plan to undo this change?

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

89–94

This can still be re-used by moving into isInvariantBreak (you can give it access to CDM by making it non-static).

96–97

This looks flipped to me, should probably say if (IsInvariantBreak) return;.

[Charusso]

• Fix.
• Document NoteTag.

[Charusso]

In D63915#1568166, @Szelethus wrote:

This checker seems to only check LLVM functions, but doesn't check whether these methods lie in the LLVM namespace. Is this intended?

Thanks for the reviews! They are not in the llvm namespace.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

Thanks! Copy-pasted, just that patch produce diagnostics as notes.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

Optional<> stands for optional values, that is why it is made. @NoQ tried to avoid it, but I believe if someone does not use it for an optional value, that break some kind of unspoken standard.

clang/test/Analysis/return-value-guaranteed.cpp

91

Whoops, too heavy copy-pasting.

[NoQ]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

Let's change to don't emit any warnings then.

[Charusso]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

I think an APIModeling could not be too much thing, most of the stuff around it is not commented out what they do. But as @Szelethus really wanted to inject that, I cannot say no to a copy-paste.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

Well, I am here at 2:1 against Optional<>, so I think it is your decision.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

96–97

It is the Optional<> checking, whether we could obtain the value. I really wanted to write !hasValue(), but no one use that, so it is some kind of unspoken standard to just ! it.

[xazax.hun]

Some nits inline, note that this was just a partial review.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

15

Is DriverDiagnostic used for something?

39

Maybe this map can be const?

66

Do you need the cast here?

70

CXXMethodDecl::getQualifiedNameAsString is not doing what you want here?

[Szelethus]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

Some are, but saying something along the lines of "most of these are enabled by default by the Driver" would've been more correct, but yea, let's leave this for another day.

[Charusso]

• More fix.

[Charusso]

Thanks for the reviews! The remaining question is: do we want to use Optional<> in the CallDescriptionMap::lookup()?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

97–101

Well, okay. Something like that is supposed to be correct for future developments:

// Checkers within APIModeling package are model APIs and enabled by the core    
// or through checker dependencies, so one should not enable/disable them by     
// hand (unless they cause a crash, but that will cause dependent checkers to    
// be implicitly disabled).                                                      
// They do not emit any warnings, just suppress infeasible paths.

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

15

I thought, but definitely not, good catch!

70

We want to drop the namespaces for better readability.

89–94

Well, sadly not. In both of the checks it is used inside the call, so you cannot just remove it. I really wanted to make it as simple as possible, but you know, "not simpler".

[NoQ]

I think i like it now!

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

I'd rather leave the original code as is and think more deeply about adding support for Optional<const T &> in the future.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

229 ↗

(On Diff #207862)

"Allow BugReporter to omit the note from the report if it would make the displayed bug path significantly shorter."

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

70

Yeah, i think it's important to display exactly what we match for.

We might eventually do some sort of CallDescription.explain() (and then maybe even CallDescriptionMap.explain(Call)) for that purpose.

89–94

Aha, ok, right!

96–97

Mmm. Aha. Ok. Indeed. Sry^^

I was thinking about simply err-ing towards "it's not a break" when we don't know for sure that it's a break, because in this case we have no problems with taking the branch that we want.

But your code seems to be more careful and i like it :)

155

Hmm. It is enough to set IsPrunable to false; once you do that, there's actually no need to mark the stack frame as interesting. I guess this wasn't really necessary.

[Szelethus]

This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, looks great!

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1119 ↗

(On Diff #207366)

Are you aware of this thread? http://lists.llvm.org/pipermail/cfe-dev/2018-July/058427.html

[Charusso]

• Done.

[Charusso]

Thanks for the reviews!

In D63915#1569410, @Szelethus wrote:

This checker isn't in alpha -- did you evaluate it on LLVM? Other than that, looks great!

Yes, it is made for LLVM and tested out 4 times. Thanks!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 
 def LocalizabilityAlpha : Package<"localizability">, ParentPackage<CocoaAlpha>;
 def LocalizabilityOptIn : Package<"localizability">, ParentPackage<CocoaOptIn>;
 
 def MPI : Package<"mpi">, ParentPackage<OptIn>;
 
 def LLVM : Package<"llvm">;
 def LLVMAlpha : Package<"llvm">, ParentPackage<Alpha>;
 
-// The APIModeling package is for checkers that model APIs and don't perform
-// any diagnostics. These checkers are always turned on; this package is
-// intended for API modeling that is not controlled by the target triple.
-def APIModeling : Package<"apiModeling">, Hidden;
+// The APIModeling package is for checkers that model APIs. These checkers are
+// always turned on; this package is intended for API modeling that is not
+// controlled by the target triple.
+def APIModeling : Package<"apiModeling">;
 def GoogleAPIModeling : Package<"google">, ParentPackage<APIModeling>, Hidden;
 
 def Debug : Package<"debug">, Hidden;
 
 def CloneDetectionAlpha : Package<"clone">, ParentPackage<Alpha>;
 
 def NonDeterminismAlpha : Package<"nondeterminism">, ParentPackage<Alpha>;
 

 } // end "nullability"
 
 //===----------------------------------------------------------------------===//
 // APIModeling.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = APIModeling in {
 
+def ReturnValueChecker : Checker<"ReturnValue">,
+  HelpText<"Model the guaranteed boolean return value of function calls">,
+  CheckerOptions<[
+    CmdLineOption<String,
+                  "Calls",
+                  "A semicolon separated list of tuples, each consists of a "
+                  "return value, call name and an optional class name "
+                  "separated by colons: \"return value:call:class;return...\". "
+                  "It ensures the boolean return value of each function call.",
+                  "\"\"",
+                  Released>,
+  ]>,
+  Documentation<HasDocumentation>;
+
 def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   HelpText<"Improve modeling of the C standard library functions">,
   Documentation<NotDocumented>;
 
 def TrustNonnullChecker : Checker<"TrustNonnull">,
   HelpText<"Trust that returns from framework methods annotated with _Nonnull "
            "are not null">,
   Documentation<NotDocumented>;

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   PointerIterationChecker.cpp
   PointerSortingChecker.cpp
   PointerSubChecker.cpp
   PthreadLockChecker.cpp
   RetainCountChecker/RetainCountChecker.cpp
   RetainCountChecker/RetainCountDiagnostics.cpp
   ReturnPointerRangeChecker.cpp
   ReturnUndefChecker.cpp
+  ReturnValueChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
   SimpleStreamChecker.cpp
   SmartPtrModeling.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
   StreamChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

+//===- ReturnValueChecker - Applies guaranteed return values ----*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines ReturnValueChecker, which checks for calls with guaranteed
+// boolean return value. It ensures the return value of each function call.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Basic/IdentifierTable.h"
+#include "clang/Driver/DriverDiagnostic.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+struct CallTy {
+  bool TruthValue;
+  StringRef Name;
+  Optional<StringRef> Class;
+};
+}
+
+namespace {
+class ReturnValueChecker : public Checker<check::PostCall> {
+public:
+  void checkPostCall(const CallEvent &CE, CheckerContext &C) const;
+  void setCalls(StringRef Calls);
+
+private:
+  SmallVector<CallTy, 13> CallVector;
+};
+} // namespace
+
+void ReturnValueChecker::checkPostCall(const CallEvent &CE,
+                                       CheckerContext &C) const {
+  // Match the calls.
+  const CallTy *Call = nullptr;
+  if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
+    for (const auto &CurrentCall : CallVector) {
+      // Match by call name.
+      if (!II->getName().equals_lower(CurrentCall.Name))
+        continue;
+
+      // We are in the mode when no class specified.
+      if (!CurrentCall.Class.hasValue()) {
+        Call = &CurrentCall;
+        break;
+      }
+
+      // Match by class name.
+      if (Optional<StringRef> ClassName = CurrentCall.Class)
+        if (const auto *MD = dyn_cast<CXXMethodDecl>(CE.getDecl()))
+          if (!MD->getParent()->getName().equals_lower(*ClassName))
+            continue;
+
+      Call = &CurrentCall;
+      break;
+    }
+  }
+
+  if (!Call)
+    return;
+
+  // Create a note.
+  const NoteTag *CallTag = C.getNoteTag([Call](BugReport &BR) -> std::string {
+    SmallString<128> Msg;
+    llvm::raw_svector_ostream Out(Msg);
+
+    Out << '\'' << Call->Class << "::" << Call->Name << "' always return "
+        << (Call->TruthValue ? "true" : "false");
+
+    return Out.str();
+  });
+
+  // Set the return value with the note.
+  ProgramStateRef State = C.getState();
+  SVal RetV = CE.getReturnValue();
+  Optional<DefinedOrUnknownSVal> RetDV = RetV.getAs<DefinedOrUnknownSVal>();
+
+  State = State->assume(*RetDV, Call->TruthValue);
+  C.addTransition(State, CallTag);
+}
+
+void ReturnValueChecker::setCalls(StringRef Calls) {
+  if (Calls == "\"\"" || Calls.size() == 0)
+    return;
+
+  SmallVector<StringRef, 13> RawTupleVector;
+  Calls.split(RawTupleVector, ';');
+
+  for (const auto &RawTuple : RawTupleVector) {
+    SmallVector<StringRef, 3> Tuple;
+    RawTuple.split(Tuple, ':');
+
+    CallTy Call;
+    Call.TruthValue = Tuple[0].trim().equals("true");
+    Call.Name = Tuple[1].trim();
+    Call.Class = Tuple.size() == 3 ? Tuple[2].trim() : Optional<StringRef>();
+
+    CallVector.push_back(Call);
+  }
+}
+
+void ento::registerReturnValueChecker(CheckerManager &Mgr) {
+  ReturnValueChecker *RVC = Mgr.registerChecker<ReturnValueChecker>();
+
+  // These are known in the LLVM project.
+  StringRef LLVMCalls = "true:printError:AsmParser;"
+                        "true:Error:LLLexer;"
+                        "true:Error:MCAsmParser;"
+                        "true:error:MIParser";
+  RVC->setCalls(LLVMCalls);
+
+  StringRef OptionalCalls =
+      Mgr.getAnalyzerOptions().getCheckerStringOption(RVC, "Calls");
+  RVC->setCalls(OptionalCalls);
+}
+
+bool ento::shouldRegisterReturnValueChecker(const LangOptions &LO) {
+  return true;
+}

clang/test/Analysis/analyzer-config.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=debug.ConfigDumper > %t 2>&1
 // RUN: FileCheck --input-file=%t %s --match-full-lines
 
 // CHECK: [config]
 // CHECK-NEXT: add-pop-up-notes = true
 // CHECK-NEXT: aggressive-binary-operation-simplification = false
 // CHECK-NEXT: alpha.clone.CloneChecker:IgnoredFilesPattern = ""
 // CHECK-NEXT: alpha.clone.CloneChecker:MinimumCloneComplexity = 50
 // CHECK-NEXT: alpha.clone.CloneChecker:ReportNormalClones = true
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtExec = 0x04
 // CHECK-NEXT: alpha.security.MmapWriteExec:MmapProtRead = 0x01
+// CHECK-NEXT: apiModeling.ReturnValue:Calls = ""
 // CHECK-NEXT: avoid-suppressing-null-argument-paths = false
 // CHECK-NEXT: c++-allocator-inlining = true
 // CHECK-NEXT: c++-container-inlining = false
 // CHECK-NEXT: c++-inlining = destructors
 // CHECK-NEXT: c++-shared_ptr-inlining = false
 // CHECK-NEXT: c++-stdlib-inlining = true
 // CHECK-NEXT: c++-temp-dtor-inlining = true
 // CHECK-NEXT: c++-template-inlining = true

 // CHECK-NEXT: stable-report-filename = false
 // CHECK-NEXT: suppress-c++-stdlib = true
 // CHECK-NEXT: suppress-inlined-defensive-checks = true
 // CHECK-NEXT: suppress-null-return-paths = true
 // CHECK-NEXT: unix.DynamicMemoryModeling:Optimistic = false
 // CHECK-NEXT: unroll-loops = false
 // CHECK-NEXT: widen-loops = false
 // CHECK-NEXT: [stats]
-// CHECK-NEXT: num-entries = 85
+// CHECK-NEXT: num-entries = 86

clang/test/Analysis/return-value-guaranteed.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -analyzer-checker=apiModeling.ReturnValue \
+// RUN:  -analyzer-config apiModeling.ReturnValue:Calls="true:error" \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -analyzer-checker=apiModeling.ReturnValue \
+// RUN:  -analyzer-config \
+// RUN:   apiModeling.ReturnValue:Calls="true:error;false:error:ErrorHandler" \
+// RUN:  -verify %s
+
+// FIXME: expected-no-diagnostics
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_warnIfReached();
+
+
+struct Foo { int Field; };
+bool error();
+bool problem();
+void doSomething();
+
+// We predefine the return value of 'error()' to 'true' and we cannot take the
+// false-branches where error would occur.
+namespace test_calls {
+bool parseFoo(Foo &F) {
+  if (problem())
+    return error();
+
+  F.Field = 0;
+  return false;
+}
+
+bool parseFile() {
+  clang_analyzer_eval(error() == true); // FIXME: xpected-warning{{TRUE}}
+
+  Foo F;
+  if (parseFoo(F))
+    return true;
+
+  if (F.Field == 0) {
+    // no-warning: "The left operand of '==' is a garbage value" was here.
+    doSomething();
+  }
+
+  return false;
+}
+} // namespace test_calls
+
+namespace test_classes {
+struct ErrorHandler {
+  static bool error();
+};
+
+bool parseFoo(Foo &F) {
+  if (problem())
+    return error();
+
+  F.Field = 0;
+  return ErrorHandler::error();
+}
+
+bool parseFile() {
+  Foo F;
+  if (parseFoo(F))
+    return true;
+
+  if (F.Field == 0) {
+    // no-warning: "The left operand of '==' is a garbage value" was here.
+    doSomething();
+  }
+
+  return false;
+}
+} // namespace test_classes
+
+void test_note() {
+  clang_analyzer_eval(error() == true); // FIXME: xpected-warning{{TRUE}}
+  if (error())
+    (void)(1 / !error());
+}