[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

37 ↗

(On Diff #206983)

It is blind.

82 ↗

(On Diff #206983)

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

31–37 ↗

(On Diff #207135)

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.

81 ↗

(On Diff #207135)

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.

89 ↗

(On Diff #207135)

"returns"

98 ↗

(On Diff #207135)

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

20 ↗

(On Diff #207135)

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

20 ↗

(On Diff #207135)

(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

98 ↗

(On Diff #207135)

Sometimes it failed with live tests in the wild.

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

20 ↗

(On Diff #207135)

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

52 ↗

(On Diff #207366)

Nope. NoReturnFunctionChecker only reacts on C functions.

98 ↗

(On Diff #207135)

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

14 ↗

(On Diff #207366)

Do we still need this include?

39 ↗

(On Diff #207366)

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

82 ↗

(On Diff #207366)

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

89–91 ↗

(On Diff #207366)

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

39 ↗

(On Diff #207366)

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

82 ↗

(On Diff #207366)

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.

98 ↗

(On Diff #207135)

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

[NoQ]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

82 ↗

(On Diff #207366)

Just use CXXMethodDecl::getParent().

[Charusso]

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

[Charusso]

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

82 ↗

(On Diff #207366)

Thanks, I really wanted to have a generic solution.

[NoQ]

This is starting to look great, thanks!

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

96 ↗

(On Diff #207458)

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.

120 ↗

(On Diff #207458)

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

71 ↗

(On Diff #207458)

auto is encouraged here.

86 ↗

(On Diff #207458)

Either cast<> or check for null.

116 ↗

(On Diff #207458)

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

120 ↗

(On Diff #207458)

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

98–100 ↗

(On Diff #207668)

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

107 ↗

(On Diff #207668)

Something's wrong with formatting.

109 ↗

(On Diff #207668)

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.

145 ↗

(On Diff #207668)

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.

145–157 ↗

(On Diff #207668)

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

170 ↗

(On Diff #207668)

Something's wrong with formatting.

[Charusso]

• Fix.
• Refactor.

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

98–100 ↗

(On Diff #207668)

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

clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp

145 ↗

(On Diff #207668)

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

98–100 ↗

(On Diff #207668)

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

90 ↗

(On Diff #207703)

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

[NoQ]

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

98–100 ↗

(On Diff #207668)

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

88–93 ↗

(On Diff #207703)

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

95–96 ↗

(On Diff #207703)

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

98–100 ↗

(On Diff #207668)

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

90 ↗

(On Diff #207703)

Whoops, too heavy copy-pasting.

[NoQ]

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

98–100 ↗

(On Diff #207668)

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

[Charusso]

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

98–100 ↗

(On Diff #207668)

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

95–96 ↗

(On Diff #207703)

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

14 ↗

(On Diff #207851)

Is DriverDiagnostic used for something?

38 ↗

(On Diff #207851)

Maybe this map can be const?

65 ↗

(On Diff #207851)

Do you need the cast here?

69 ↗

(On Diff #207851)

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

[Szelethus]

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

98–100 ↗

(On Diff #207668)

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

98–100 ↗

(On Diff #207668)

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

88–93 ↗

(On Diff #207703)

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

14 ↗

(On Diff #207851)

I thought, but definitely not, good catch!

69 ↗

(On Diff #207851)

We want to drop the namespaces for better readability.

[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

88–93 ↗

(On Diff #207703)

Aha, ok, right!

95–96 ↗

(On Diff #207703)

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

69 ↗

(On Diff #207851)

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.

154 ↗

(On Diff #207862)

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!

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 } // end "nullability"
 
 //===----------------------------------------------------------------------===//
 // APIModeling.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = APIModeling in {
 
+def ReturnValueChecker : Checker<"ReturnValue">,
+  HelpText<"Model the guaranteed boolean return value of function calls">,
+  Documentation<NotDocumented>;
+
 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>;

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

public:
   }
 
   /// Emit the diagnostics report.
   void emitReport(std::unique_ptr<BugReport> R) {
     Changed = true;
     Eng.getBugReporter().emitReport(std::move(R));
   }
 
-
   /// Produce a program point tag that displays an additional path note
   /// to the user. This is a lightweight alternative to the
   /// BugReporterVisitor mechanism: instead of visiting the bug report
   /// node-by-node to restore the sequence of events that led to discovering
   /// a bug, you can add notes as you add your transitions.
-  const NoteTag *getNoteTag(NoteTag::Callback &&Cb) {
-    return Eng.getNoteTags().makeNoteTag(std::move(Cb));
+  ///
+  /// @param Cb Callback with 'BugReporterContext &, BugReport &' parameters.
+  /// @param IsPrunable Whether the note is prunable. It allows BugReporter
+  ///        to omit the note from the report if it would make the displayed
+  ///        bug path significantly shorter.
+  const NoteTag *getNoteTag(NoteTag::Callback &&Cb, bool IsPrunable = false) {
+    return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable);
   }
 
   /// A shorthand version of getNoteTag that doesn't require you to accept
   /// the BugReporterContext arguments when you don't need it.
-  const NoteTag *getNoteTag(std::function<std::string(BugReport &)> &&Cb) {
+  ///
+  /// @param Cb Callback only with 'BugReport &' parameter.
+  /// @param IsPrunable Whether the note is prunable. It allows BugReporter
+  ///        to omit the note from the report if it would make the displayed
+  ///        bug path significantly shorter.
+  const NoteTag *getNoteTag(std::function<std::string(BugReport &)> &&Cb,
+                            bool IsPrunable = false) {
     return getNoteTag(
-        [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); });
+        [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); },
+        IsPrunable);
   }
 
-
   /// Returns the word that should be used to refer to the declaration
   /// in the report.
   StringRef getDeclDescription(const Decl *D);
 
   /// Get the declaration of the called function (path-sensitive).
   const FunctionDecl *getCalleeDecl(const CallExpr *CE) const;
 
   /// Get the name of the called function (path-sensitive).

cfe/trunk/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

cfe/trunk/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/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.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"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class ReturnValueChecker : public Checker<check::PostCall, check::EndFunction> {
+public:
+  // It sets the predefined invariant ('CDM') if the current call not break it.
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+
+  // It reports whether a predefined invariant ('CDM') is broken.
+  void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
+
+private:
+  // The pairs are in the following form: {{{class, call}}, return value}
+  const CallDescriptionMap<bool> CDM = {
+      // These are known in the LLVM project: 'Error()'
+      {{{"ARMAsmParser", "Error"}}, true},
+      {{{"HexagonAsmParser", "Error"}}, true},
+      {{{"LLLexer", "Error"}}, true},
+      {{{"LLParser", "Error"}}, true},
+      {{{"MCAsmParser", "Error"}}, true},
+      {{{"MCAsmParserExtension", "Error"}}, true},
+      {{{"TGParser", "Error"}}, true},
+      {{{"X86AsmParser", "Error"}}, true},
+      // 'TokError()'
+      {{{"LLParser", "TokError"}}, true},
+      {{{"MCAsmParser", "TokError"}}, true},
+      {{{"MCAsmParserExtension", "TokError"}}, true},
+      {{{"TGParser", "TokError"}}, true},
+      // 'error()'
+      {{{"MIParser", "error"}}, true},
+      {{{"WasmAsmParser", "error"}}, true},
+      {{{"WebAssemblyAsmParser", "error"}}, true},
+      // Other
+      {{{"AsmParser", "printError"}}, true}};
+};
+} // namespace
+
+static std::string getName(const CallEvent &Call) {
+  std::string Name = "";
+  if (const auto *MD = dyn_cast<CXXMethodDecl>(Call.getDecl()))
+    if (const CXXRecordDecl *RD = MD->getParent())
+      Name += RD->getNameAsString() + "::";
+
+  Name += Call.getCalleeIdentifier()->getName();
+  return Name;
+}
+
+// The predefinitions ('CDM') could break due to the ever growing code base.
+// Check for the expected invariants and see whether they apply.
+static Optional<bool> isInvariantBreak(bool ExpectedValue, SVal ReturnV,
+                                       CheckerContext &C) {
+  auto ReturnDV = ReturnV.getAs<DefinedOrUnknownSVal>();
+  if (!ReturnDV)
+    return None;
+
+  if (ExpectedValue)
+    return C.getState()->isNull(*ReturnDV).isConstrainedTrue();
+
+  return C.getState()->isNull(*ReturnDV).isConstrainedFalse();
+}
+
+void ReturnValueChecker::checkPostCall(const CallEvent &Call,
+                                       CheckerContext &C) const {
+  const bool *RawExpectedValue = CDM.lookup(Call);
+  if (!RawExpectedValue)
+    return;
+
+  SVal ReturnV = Call.getReturnValue();
+  bool ExpectedValue = *RawExpectedValue;
+  Optional<bool> IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, C);
+  if (!IsInvariantBreak)
+    return;
+
+  // If the invariant is broken it is reported by 'checkEndFunction()'.
+  if (*IsInvariantBreak)
+    return;
+
+  std::string Name = getName(Call);
+  const NoteTag *CallTag = C.getNoteTag(
+      [Name, ExpectedValue](BugReport &) -> std::string {
+        SmallString<128> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+
+        Out << '\'' << Name << "' returns "
+            << (ExpectedValue ? "true" : "false");
+        return Out.str();
+      },
+      /*IsPrunable=*/true);
+
+  ProgramStateRef State = C.getState();
+  State = State->assume(ReturnV.castAs<DefinedOrUnknownSVal>(), ExpectedValue);
+  C.addTransition(State, CallTag);
+}
+
+void ReturnValueChecker::checkEndFunction(const ReturnStmt *RS,
+                                          CheckerContext &C) const {
+  if (!RS || !RS->getRetValue())
+    return;
+
+  // We cannot get the caller in the top-frame.
+  const StackFrameContext *SFC = C.getStackFrame();
+  if (C.getStackFrame()->inTopFrame())
+    return;
+
+  ProgramStateRef State = C.getState();
+  CallEventManager &CMgr = C.getStateManager().getCallEventManager();
+  CallEventRef<> Call = CMgr.getCaller(SFC, State);
+  if (!Call)
+    return;
+
+  const bool *RawExpectedValue = CDM.lookup(*Call);
+  if (!RawExpectedValue)
+    return;
+
+  SVal ReturnV = State->getSVal(RS->getRetValue(), C.getLocationContext());
+  bool ExpectedValue = *RawExpectedValue;
+  Optional<bool> IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, C);
+  if (!IsInvariantBreak)
+    return;
+
+  // If the invariant is appropriate it is reported by 'checkPostCall()'.
+  if (!*IsInvariantBreak)
+    return;
+
+  std::string Name = getName(*Call);
+  const NoteTag *CallTag = C.getNoteTag(
+      [Name, ExpectedValue](BugReport &BR) -> std::string {
+        SmallString<128> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+
+        // The following is swapped because the invariant is broken.
+        Out << '\'' << Name << "' returns "
+            << (ExpectedValue ? "false" : "true");
+
+        return Out.str();
+      },
+      /*IsPrunable=*/false);
+
+  C.addTransition(State, CallTag);
+}
+
+void ento::registerReturnValueChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<ReturnValueChecker>();
+}
+
+bool ento::shouldRegisterReturnValueChecker(const LangOptions &LO) {
+  return true;
+}

cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp

if (Optional<BlockEdge> BE = P.getAs<BlockEdge>()) {
     CFGElement BlockFront = BE->getBlock()->front();
     if (auto StmtElt = BlockFront.getAs<CFGStmt>()) {
       return PathDiagnosticLocation(StmtElt->getStmt()->getBeginLoc(), SMng);
     } else if (auto NewAllocElt = BlockFront.getAs<CFGNewAllocator>()) {
       return PathDiagnosticLocation(
           NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
     }
     llvm_unreachable("Unexpected CFG element at front of block");
+  } else if (Optional<FunctionExitPoint> FE = P.getAs<FunctionExitPoint>()) {
+    return PathDiagnosticLocation(FE->getStmt(), SMng,
+                                  FE->getLocationContext());
   } else {
     llvm_unreachable("Unexpected ProgramPoint");
   }
 
   return PathDiagnosticLocation(S, SMng, P.getLocationContext());
 }
 
 static const LocationContext *

cfe/trunk/test/Analysis/return-value-guaranteed.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,apiModeling.ReturnValue \
+// RUN:  -analyzer-output=text -verify=class %s
+
+struct Foo { int Field; };
+bool problem();
+void doSomething();
+
+// We predefined the return value of 'MCAsmParser::Error' as true and we cannot
+// take the false-branches which leads to a "garbage value" false positive.
+namespace test_classes {
+struct MCAsmParser {
+  static bool Error();
+};
+
+bool parseFoo(Foo &F) {
+  if (problem()) {
+    // class-note@-1 {{Assuming the condition is false}}
+    // class-note@-2 {{Taking false branch}}
+    return MCAsmParser::Error();
+  }
+
+  F.Field = 0;
+  // class-note@-1 {{The value 0 is assigned to 'F.Field'}}
+  return !MCAsmParser::Error();
+  // class-note@-1 {{'MCAsmParser::Error' returns true}}
+}
+
+bool parseFile() {
+  Foo F;
+  if (parseFoo(F)) {
+    // class-note@-1 {{Calling 'parseFoo'}}
+    // class-note@-2 {{Returning from 'parseFoo'}}
+    // class-note@-3 {{Taking false branch}}
+    return true;
+  }
+
+  if (F.Field == 0) {
+    // class-note@-1 {{Field 'Field' is equal to 0}}
+    // class-note@-2 {{Taking true branch}}
+
+    // no-warning: "The left operand of '==' is a garbage value" was here.
+    doSomething();
+  }
+
+  (void)(1 / F.Field);
+  // class-warning@-1 {{Division by zero}}
+  // class-note@-2 {{Division by zero}}
+  return false;
+}
+} // namespace test_classes
+
+
+// We predefined 'MCAsmParser::Error' as returning true, but now it returns
+// false, which breaks our invariant. Test the notes.
+namespace test_break {
+struct MCAsmParser {
+  static bool Error() {
+    return false; // class-note {{'MCAsmParser::Error' returns false}}
+  }
+};
+
+bool parseFoo(Foo &F) {
+  if (problem()) {
+    // class-note@-1 {{Assuming the condition is false}}
+    // class-note@-2 {{Taking false branch}}
+    return !MCAsmParser::Error();
+  }
+
+  F.Field = 0;
+  // class-note@-1 {{The value 0 is assigned to 'F.Field'}}
+  return MCAsmParser::Error();
+  // class-note@-1 {{Calling 'MCAsmParser::Error'}}
+  // class-note@-2 {{Returning from 'MCAsmParser::Error'}}
+}
+
+bool parseFile() {
+  Foo F;
+  if (parseFoo(F)) {
+    // class-note@-1 {{Calling 'parseFoo'}}
+    // class-note@-2 {{Returning from 'parseFoo'}}
+    // class-note@-3 {{Taking false branch}}
+    return true;
+  }
+
+  (void)(1 / F.Field);
+  // class-warning@-1 {{Division by zero}}
+  // class-note@-2 {{Division by zero}}
+  return false;
+}
+} // namespace test_classes