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

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365103: [analyzer] ReturnValueChecker: Model the guaranteed 
boolean return value of… (authored by Charusso, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63915?vs=207939=207942#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D63915

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  cfe/trunk/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  cfe/trunk/test/Analysis/return-value-guaranteed.cpp

Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
@@ -219,24 +219,34 @@
 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 &) {
-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 &, 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 &) {
+  ///
+  /// @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 &,
+bool IsPrunable = false) {
 return getNoteTag(
-[Cb](BugReporterContext &, BugReport ) { return Cb(BR); });
+[Cb](BugReporterContext &, BugReport ) { return Cb(BR); },
+IsPrunable);
   }
 
-
   /// Returns the word that should be used to refer to the declaration
   /// in the report.
   StringRef getDeclDescription(const Decl *D);
Index: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
===
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -274,6 +274,10 @@
 
 let ParentPackage = APIModeling in {
 
+def ReturnValueChecker : Checker<"ReturnValue">,
+  HelpText<"Model the guaranteed boolean return value of function calls">,
+  Documentation;
+
 def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   HelpText<"Improve modeling of the C standard library functions">,
   Documentation;
Index: cfe/trunk/test/Analysis/return-value-guaranteed.cpp
===
--- cfe/trunk/test/Analysis/return-value-guaranteed.cpp
+++ cfe/trunk/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,91 @@
+// 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 ) {
+  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) {
+// 

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 207939.
Charusso marked 9 inline comments as done.
Charusso added a comment.

- Done.


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

https://reviews.llvm.org/D63915

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/return-value-guaranteed.cpp

Index: clang/test/Analysis/return-value-guaranteed.cpp
===
--- /dev/null
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,91 @@
+// 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 ) {
+  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 ) {
+  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
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -780,6 +780,9 @@
   NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
 }
 llvm_unreachable("Unexpected CFG element at front of block");
+  } else if (Optional FE = P.getAs()) {
+return PathDiagnosticLocation(FE->getStmt(), SMng,
+  FE->getLocationContext());
   } else {
 llvm_unreachable("Unexpected ProgramPoint");
   }
Index: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
@@ -0,0 +1,170 @@
+//===- 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 

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

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!


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > Szelethus wrote:
> > > > > Charusso wrote:
> > > > > > NoQ wrote:
> > > > > > > I hope `T` never gets too expensive to copy. The ideal return 
> > > > > > > value here is `Optional` 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?
> > > > > > Optional is working and used widely, I like that.
> > > > > Why do we need the optional AND the pointer? How about we just return 
> > > > > with a nullptr if we fail to find the call?
> > > > `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.
> > > Well, that'd be the original code.
> > > 
> > > > I do not like `Optional` anymore.
> > > 
> > > @Charusso, do you still plan to undo this change?
> > Well, I am here at 2:1 against `Optional<>`, so I think it is your decision.
> I'd rather leave the original code as is and think more deeply about adding 
> support for `Optional` in the future.
Are you aware of this thread? 
http://lists.llvm.org/pipermail/cfe-dev/2018-July/058427.html


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

I think i like it now!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

Charusso wrote:
> NoQ wrote:
> > Charusso wrote:
> > > Szelethus wrote:
> > > > Charusso wrote:
> > > > > NoQ wrote:
> > > > > > I hope `T` never gets too expensive to copy. The ideal return value 
> > > > > > here is `Optional` 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?
> > > > > Optional is working and used widely, I like that.
> > > > Why do we need the optional AND the pointer? How about we just return 
> > > > with a nullptr if we fail to find the call?
> > > `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.
> > Well, that'd be the original code.
> > 
> > > I do not like `Optional` anymore.
> > 
> > @Charusso, do you still plan to undo this change?
> Well, I am here at 2:1 against `Optional<>`, so I think it is your decision.
I'd rather leave the original code as is and think more deeply about adding 
support for `Optional` in the future.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:229
+  /// @param Cb Callback with 'BugReporterContext &, BugReport &' parameters.
+  /// @param IsPrunable Whether the note is prunable.
+  const NoteTag *getNoteTag(NoteTag::Callback &, bool IsPrunable = false) {

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



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:88-93
+  Optional RawExpectedValue = CDM.lookup(Call);
+  if (!RawExpectedValue)
+return;
+
+  SVal ReturnV = Call.getReturnValue();
+  bool ExpectedValue = **RawExpectedValue;

Charusso wrote:
> NoQ wrote:
> > This can still be re-used by moving into `isInvariantBreak` (you can give 
> > it access to `CDM` by making it non-static).
> 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".
Aha, ok, right!



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:95-96
+  Optional IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, 
C);
+  if (!IsInvariantBreak)
+return;
+

Charusso wrote:
> NoQ wrote:
> > This looks flipped to me, should probably say `if (IsInvariantBreak) 
> > return;`.
> 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.
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 :)



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:69
+  Name += Call.getCalleeIdentifier()->getName();
+  return Name;
+}

Charusso wrote:
> xazax.hun wrote:
> > `CXXMethodDecl::getQualifiedNameAsString` is not doing what you want here?
> We want to drop the namespaces for better readability.
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.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:154
+
+BR.markInteresting(SFC);
+

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.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

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




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// 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.

Szelethus wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > NoQ wrote:
> > > > > Szelethus wrote:
> > > > > > Charusso wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > 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?).
> > > > > > > Well, I have removed it instead. Makes no sense, you are right.
> > > > > > 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).
> > > > > > ```
> > > > > 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.
> > > > Thanks! Copy-pasted, just that patch produce diagnostics as notes.
> > > Let's change to `don't emit any warnings` then.
> > 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.
> 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.
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.
```



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:88-93
+  Optional RawExpectedValue = CDM.lookup(Call);
+  if (!RawExpectedValue)
+return;
+
+  SVal ReturnV = Call.getReturnValue();
+  bool ExpectedValue = **RawExpectedValue;

NoQ wrote:
> This can still be re-used by moving into `isInvariantBreak` (you can give it 
> access to `CDM` by making it non-static).
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".



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:14
+
+#include "clang/Driver/DriverDiagnostic.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"

xazax.hun wrote:
> Is DriverDiagnostic used for something?
I thought, but definitely not, good catch!



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:69
+  Name += Call.getCalleeIdentifier()->getName();
+  return Name;
+}

xazax.hun wrote:
> `CXXMethodDecl::getQualifiedNameAsString` is not doing what you want here?
We want to drop the namespaces for better readability.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 207862.
Charusso marked 12 inline comments as done.
Charusso added a comment.

- More fix.


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

https://reviews.llvm.org/D63915

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -33,11 +33,11 @@
 Impl(std::move(Data)) {}
 
   const bool *lookup(const CallEvent ) {
-const bool *Result = Impl.lookup(Call);
+Optional Result = Impl.lookup(Call);
 // If it's a function we expected to find, remember that we've found it.
-if (Result && *Result)
+if (Result && *Result && **Result)
   ++Found;
-return Result;
+return *Result;
   }
 
   // Fail the test if we haven't found all the true-calls we were looking for.
Index: clang/test/Analysis/return-value-guaranteed.cpp
===
--- /dev/null
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,91 @@
+// 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 ) {
+  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 ) {
+  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
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -780,6 +780,9 @@
   NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
 }
 llvm_unreachable("Unexpected CFG element at front of block");
+  } else if (Optional FE = P.getAs()) {
+return PathDiagnosticLocation(FE->getStmt(), SMng,
+  FE->getLocationContext());
   } else {
 llvm_unreachable("Unexpected ProgramPoint");
   }
Index: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
===
--- /dev/null
+++ 

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// 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.

Charusso wrote:
> NoQ wrote:
> > Charusso wrote:
> > > NoQ wrote:
> > > > Szelethus wrote:
> > > > > Charusso wrote:
> > > > > > Szelethus wrote:
> > > > > > > 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?).
> > > > > > Well, I have removed it instead. Makes no sense, you are right.
> > > > > 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).
> > > > > ```
> > > > 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.
> > > Thanks! Copy-pasted, just that patch produce diagnostics as notes.
> > Let's change to `don't emit any warnings` then.
> 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.
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.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

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




Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:14
+
+#include "clang/Driver/DriverDiagnostic.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"

Is DriverDiagnostic used for something?



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:38
+  // The pairs are in the following form: {{{class, call}}, return value}
+  CallDescriptionMap CDM = {
+  // These are known in the LLVM project: 'Error()'

Maybe this map can be const?



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:65
+  if (const auto *MD = dyn_cast(Call.getDecl()))
+if (const auto *RD = dyn_cast(MD->getParent()))
+  Name += RD->getNameAsString() + "::";

Do you need the cast here?



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:69
+  Name += Call.getCalleeIdentifier()->getName();
+  return Name;
+}

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 5 inline comments as done.
Charusso added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// 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.

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > Charusso wrote:
> > > > > Szelethus wrote:
> > > > > > 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?).
> > > > > Well, I have removed it instead. Makes no sense, you are right.
> > > > 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).
> > > > ```
> > > 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.
> > Thanks! Copy-pasted, just that patch produce diagnostics as notes.
> Let's change to `don't emit any warnings` then.
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.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

NoQ wrote:
> Charusso wrote:
> > Szelethus wrote:
> > > Charusso wrote:
> > > > NoQ wrote:
> > > > > I hope `T` never gets too expensive to copy. The ideal return value 
> > > > > here is `Optional` 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?
> > > > Optional is working and used widely, I like that.
> > > Why do we need the optional AND the pointer? How about we just return 
> > > with a nullptr if we fail to find the call?
> > `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.
> Well, that'd be the original code.
> 
> > I do not like `Optional` anymore.
> 
> @Charusso, do you still plan to undo this change?
Well, I am here at 2:1 against `Optional<>`, so I think it is your decision.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:95-96
+  Optional IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, 
C);
+  if (!IsInvariantBreak)
+return;
+

NoQ wrote:
> This looks flipped to me, should probably say `if (IsInvariantBreak) return;`.
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.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/Checkers/Checkers.td:98-100
+// 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.

Charusso wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > Charusso wrote:
> > > > Szelethus wrote:
> > > > > 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?).
> > > > Well, I have removed it instead. Makes no sense, you are right.
> > > 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).
> > > ```
> > 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.
> Thanks! Copy-pasted, just that patch produce diagnostics as notes.
Let's change to `don't emit any warnings` then.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

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.




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// 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.

NoQ wrote:
> Szelethus wrote:
> > Charusso wrote:
> > > Szelethus wrote:
> > > > 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?).
> > > Well, I have removed it instead. Makes no sense, you are right.
> > 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).
> > ```
> 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.
Thanks! Copy-pasted, just that patch produce diagnostics as notes.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

Szelethus wrote:
> Charusso wrote:
> > NoQ wrote:
> > > I hope `T` never gets too expensive to copy. The ideal return value here 
> > > is `Optional` 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?
> > Optional is working and used widely, I like that.
> Why do we need the optional AND the pointer? How about we just return with a 
> nullptr if we fail to find the call?
`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.



Comment at: clang/test/Analysis/return-value-guaranteed.cpp:90
+
+// no-warning: "The left operand of '==' is a garbage value" was here.
+doSomething();

Szelethus wrote:
> Was it? I just tried it out and it doesn't seem to be the case.
Whoops, too heavy copy-pasting.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 207851.
Charusso marked 8 inline comments as done.
Charusso added a comment.

- Fix.
- Document `NoteTag`.


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

https://reviews.llvm.org/D63915

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -33,11 +33,11 @@
 Impl(std::move(Data)) {}
 
   const bool *lookup(const CallEvent ) {
-const bool *Result = Impl.lookup(Call);
+Optional Result = Impl.lookup(Call);
 // If it's a function we expected to find, remember that we've found it.
-if (Result && *Result)
+if (Result && *Result && **Result)
   ++Found;
-return Result;
+return *Result;
   }
 
   // Fail the test if we haven't found all the true-calls we were looking for.
Index: clang/test/Analysis/return-value-guaranteed.cpp
===
--- /dev/null
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,91 @@
+// 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 ) {
+  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 ) {
+  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
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -780,6 +780,9 @@
   NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
 }
 llvm_unreachable("Unexpected CFG element at front of block");
+  } else if (Optional FE = P.getAs()) {
+return PathDiagnosticLocation(FE->getStmt(), SMng,
+  FE->getLocationContext());
   } else {
 llvm_unreachable("Unexpected ProgramPoint");
   }
Index: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
===
--- /dev/null
+++ 

[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/Checkers/Checkers.td:98-100
+// 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.

Szelethus wrote:
> Charusso wrote:
> > Szelethus wrote:
> > > 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?).
> > Well, I have removed it instead. Makes no sense, you are right.
> 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).
> ```
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.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

Charusso wrote:
> Szelethus wrote:
> > Charusso wrote:
> > > NoQ wrote:
> > > > I hope `T` never gets too expensive to copy. The ideal return value 
> > > > here is `Optional` 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?
> > > Optional is working and used widely, I like that.
> > Why do we need the optional AND the pointer? How about we just return with 
> > a nullptr if we fail to find the call?
> `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.
Well, that'd be the original code.

> I do not like `Optional` anymore.

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



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:88-93
+  Optional RawExpectedValue = CDM.lookup(Call);
+  if (!RawExpectedValue)
+return;
+
+  SVal ReturnV = Call.getReturnValue();
+  bool ExpectedValue = **RawExpectedValue;

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



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:95-96
+  Optional IsInvariantBreak = isInvariantBreak(ExpectedValue, ReturnV, 
C);
+  if (!IsInvariantBreak)
+return;
+

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// 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.

Charusso wrote:
> Szelethus wrote:
> > 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?).
> Well, I have removed it instead. Makes no sense, you are right.
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).
```



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

Charusso wrote:
> NoQ wrote:
> > I hope `T` never gets too expensive to copy. The ideal return value here is 
> > `Optional` 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?
> Optional is working and used widely, I like that.
Why do we need the optional AND the pointer? How about we just return with a 
nullptr if we fail to find the call?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:210
   ///the default tag for the checker will be used.
+  /// @param IsPrunable Whether the note is prunable.
   ExplodedNode *

Charusso wrote:
> It makes perfectly no sense here. Is it the mentioned "class doc"?
You're right. I meant to add it to `NoteTag`s 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.



Comment at: clang/test/Analysis/return-value-guaranteed.cpp:90
+
+// no-warning: "The left operand of '==' is a garbage value" was here.
+doSomething();

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:210
   ///the default tag for the checker will be used.
+  /// @param IsPrunable Whether the note is prunable.
   ExplodedNode *

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// 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.

Szelethus wrote:
> 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?).
Well, I have removed it instead. Makes no sense, you are right.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145
+
+  // Get the concrete return value.
+  Optional RawConcreteValue = CDM.lookup(*Call);

NoQ wrote:
> NoQ wrote:
> > I think you can squeeze all this code into one big `isInvariantBreak(Call, 
> > ReturnV)` and re-use it across both methods.
> 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.
Great advice, thanks you!


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 207703.
Charusso marked 10 inline comments as done.
Charusso added a comment.

- Fix.
- Refactor.


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

https://reviews.llvm.org/D63915

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -33,11 +33,11 @@
 Impl(std::move(Data)) {}
 
   const bool *lookup(const CallEvent ) {
-const bool *Result = Impl.lookup(Call);
+Optional Result = Impl.lookup(Call);
 // If it's a function we expected to find, remember that we've found it.
-if (Result && *Result)
+if (Result && *Result && **Result)
   ++Found;
-return Result;
+return *Result;
   }
 
   // Fail the test if we haven't found all the true-calls we were looking for.
Index: clang/test/Analysis/return-value-guaranteed.cpp
===
--- /dev/null
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,99 @@
+// 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 ) {
+  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 ) {
+  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;
+  }
+
+  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
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -780,6 +780,9 @@
   NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
 }
 llvm_unreachable("Unexpected CFG element at front of block");
+  } else if (Optional FE = P.getAs()) {
+return PathDiagnosticLocation(FE->getStmt(), SMng,
+  FE->getLocationContext());
   } else {
 

[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/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:107
+
+   // If the invariant is broken we do not return the concrete value.
+if (IsInvariantBreak)

Something's wrong with formatting.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:109
+if (IsInvariantBreak)
+  Out << " should return ";
+else

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



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145
+
+  // Get the concrete return value.
+  Optional RawConcreteValue = CDM.lookup(*Call);

NoQ wrote:
> I think you can squeeze all this code into one big `isInvariantBreak(Call, 
> ReturnV)` and re-use it across both methods.
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.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:145-157
+  // Get the concrete return value.
+  Optional RawConcreteValue = CDM.lookup(*Call);
+  if (!RawConcreteValue)
+return;
+ 
+  // Get the symbolic return value.
+  SVal ReturnV = State->getSVal(RS->getRetValue(), C.getLocationContext());

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



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:170
+
+   // The following is swapped because the invariant is broken.
+Out << '\'' << Name << "' returns "

Something's wrong with formatting.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/CheckerContext.h:228-229
   /// a bug, you can add notes as you add your transitions.
-  const NoteTag *getNoteTag(NoteTag::Callback &) {
-return Eng.getNoteTags().makeNoteTag(std::move(Cb));
+  const NoteTag *getNoteTag(NoteTag::Callback &, bool IsPrunable = false) {
+return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable);
   }

Szelethus wrote:
> 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? :)
> 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.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

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




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:98-100
+// 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.

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:228-229
   /// a bug, you can add notes as you add your transitions.
-  const NoteTag *getNoteTag(NoteTag::Callback &) {
-return Eng.getNoteTags().makeNoteTag(std::move(Cb));
+  const NoteTag *getNoteTag(NoteTag::Callback &, bool IsPrunable = false) {
+return Eng.getNoteTags().makeNoteTag(std::move(Cb), IsPrunable);
   }

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:120
+<< (Value ? "true" : "false")
+<< " according to the LLVM coding standard, but it returns "
+<< (Value ? "false" : "true");

NoQ wrote:
> 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`.
Well, we have tons of unspoken standards like: `MR, DRE, SE, RD, V` all 
makes sense, but I like your idea more.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 207668.
Charusso marked 7 inline comments as done.
Charusso added a comment.

- Create `FunctionExitPoint` diagnostics.
- Fix.


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

https://reviews.llvm.org/D63915

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -33,11 +33,11 @@
 Impl(std::move(Data)) {}
 
   const bool *lookup(const CallEvent ) {
-const bool *Result = Impl.lookup(Call);
+Optional Result = Impl.lookup(Call);
 // If it's a function we expected to find, remember that we've found it.
-if (Result && *Result)
+if (Result && *Result && **Result)
   ++Found;
-return Result;
+return *Result;
   }
 
   // Fail the test if we haven't found all the true-calls we were looking for.
Index: clang/test/Analysis/return-value-guaranteed.cpp
===
--- /dev/null
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,100 @@
+// 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 ) {
+  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 an unspoken LLVM coding standard. Test the notes.
+namespace test_break {
+struct MCAsmParser {
+  static bool Error() {
+return false; // class-note {{'MCAsmParser::Error' returns false}}
+  }
+};
+
+bool parseFoo(Foo ) {
+  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'}}
+  // class-note@-3 {{'MCAsmParser::Error' should return true but it returns false}}
+}
+
+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
Index: clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===
--- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -780,6 +780,9 @@
   NewAllocElt->getAllocatorExpr()->getBeginLoc(), SMng);
 }
 llvm_unreachable("Unexpected CFG element at front of block");
+  } else if (Optional FE = P.getAs()) {
+return 

[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/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:71
+  SVal RetV = Call.getReturnValue();
+  Optional RetDV = RetV.getAs();
+  if (!RetDV.hasValue())

`auto` is encouraged here.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

This is starting to look great, thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:96
+
+  Out << '\'' << Name << "' always returns "
+  << (Value ? "true" : "false");

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.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:120
+<< (Value ? "true" : "false")
+<< " according to the LLVM coding standard, but it returns "
+<< (Value ? "false" : "true");

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:86
+  std::string Name = "";
+  Name += dyn_cast(Call.getDecl())->getParent()->getName();
+  Name += "::";

Either `cast<>` or check for null.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:116
+
+BR.markInteresting(SFC);
+

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 207458.
Charusso added a comment.

- I do not like `Optional` anymore.
- More simple notes.


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

https://reviews.llvm.org/D63915

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -33,11 +33,11 @@
 Impl(std::move(Data)) {}
 
   const bool *lookup(const CallEvent ) {
-const bool *Result = Impl.lookup(Call);
+Optional Result = Impl.lookup(Call);
 // If it's a function we expected to find, remember that we've found it.
-if (Result && *Result)
+if (Result && *Result && **Result)
   ++Found;
-return Result;
+return *Result;
   }
 
   // Fail the test if we haven't found all the true-calls we were looking for.
Index: clang/test/Analysis/return-value-guaranteed.cpp
===
--- /dev/null
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,96 @@
+// 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 ) {
+  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' always 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 the LLVM coding standard. Test the note.
+namespace test_break {
+struct MCAsmParser {
+  static bool Error() { return false; }
+};
+
+bool parseFoo(Foo ) {
+  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' should always return true according to the LLVM coding standard, but it returns false}}
+}
+
+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
Index: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
@@ -0,0 +1,137 @@
+//===- 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 

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82
+
+Out << '\'' << Name << "' always returns "
+<< (*Value ? "true" : "false");

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Let's mention the class name as well! Maybe even the fully qualified 
> > > namespace.
> > 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.
> Just use `CXXMethodDecl::getParent()`.
Thanks, I really wanted to have a generic solution.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82
+
+Out << '\'' << Name << "' always returns "
+<< (*Value ? "true" : "false");

Charusso wrote:
> NoQ wrote:
> > Let's mention the class name as well! Maybe even the fully qualified 
> > namespace.
> 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.
Just use `CXXMethodDecl::getParent()`.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 207456.
Charusso marked 11 inline comments as done.
Charusso added a comment.

- Fix.


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

https://reviews.llvm.org/D63915

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
  clang/test/Analysis/return-value-guaranteed.cpp
  clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp

Index: clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
+++ clang/unittests/StaticAnalyzer/CallDescriptionTest.cpp
@@ -33,11 +33,11 @@
 Impl(std::move(Data)) {}
 
   const bool *lookup(const CallEvent ) {
-const bool *Result = Impl.lookup(Call);
+Optional Result = Impl.lookup(Call);
 // If it's a function we expected to find, remember that we've found it.
 if (Result && *Result)
   ++Found;
-return Result;
+return *Result;
   }
 
   // Fail the test if we haven't found all the true-calls we were looking for.
Index: clang/test/Analysis/return-value-guaranteed.cpp
===
--- /dev/null
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -0,0 +1,96 @@
+// 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 ) {
+  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 {{'test_classes::MCAsmParser::Error' always 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 the LLVM coding standard. Test the note.
+namespace test_break {
+struct MCAsmParser {
+  static bool Error() { return false; }
+};
+
+bool parseFoo(Foo ) {
+  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 {{'test_break::MCAsmParser::Error' should always return true according to the LLVM coding standard, but it returns false}}
+}
+
+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
Index: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp
@@ -0,0 +1,133 @@
+//===- 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

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

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?




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1119
 
-  const T *lookup(const CallEvent ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

NoQ wrote:
> I hope `T` never gets too expensive to copy. The ideal return value here is 
> `Optional` 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?
Optional is working and used widely, I like that.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:98
+  SVal RetV = CE.getReturnValue();
+  Optional RetDV = RetV.getAs();
+

NoQ wrote:
> 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?
After the patch finishes I will make tests and try to break it again.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:39
+  // 'Error()'
+  {{{"ARMAsmParser", "Error"}}, true},
+  {{{"HexagonAsmParser", "Error"}}, true},

NoQ wrote:
> We can play even more nicely here by requiring namespace `llvm` as well (just 
> prepend one more item to the list).
Well, I have checked 4 of them, they are in the anonymous namespace, so I will 
leave it.



Comment at: clang/lib/StaticAnalyzer/Checkers/ReturnValueChecker.cpp:82
+
+Out << '\'' << Name << "' always returns "
+<< (*Value ? "true" : "false");

NoQ wrote:
> Let's mention the class name as well! Maybe even the fully qualified 
> namespace.
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.


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/unittests/StaticAnalyzer/CallDescriptionTest.cpp:36
 
-const bool *LookupResult = CDM.lookup(*Call);
+Optional LookupResult = CDM.lookup(*Call);
 // Check that we've found the function in the map

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


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

https://reviews.llvm.org/D63915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 ) const {
+  Optional lookup(const CallEvent ) const {
 // Slow path: linear lookup.

I hope `T` never gets too expensive to copy. The ideal return value here is 
`Optional` 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 RetDV = RetV.getAs();
+

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits