[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.

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

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:309-311
+void ErrorReturnChecker::checkAccess(CheckerContext &C, ProgramStateRef State,
+                                     const Stmt *LoadS, SymbolRef CallSym,
+                                     const CalledFunctionData *CFD) const {
----------------
NoQ wrote:
> Why scan parent statements proactively given that we will reach them later anyway? Looks like you're using a wrong checker callback.
Thats one of the things I'm not super familiar with just yet. Are you referencing how a CFGBlock is built up? I remember trying to solve a similar problem (dig writes/reads out of `Stmt *`s) when working on the reaching definitions calculator, and I question my methodology as well.

test.cpp:
```lang=c++
int getInt();

void different_assignments() {
  int i = getInt(); // Return value to check.

  if (static_cast<bool>(i < 7))
    ;
}
```

AST:
```
`-FunctionDecl 0xaf6ff0 <line:3:1, line:8:1> line:3:6 different_assignments 'void ()'
  `-CompoundStmt 0xaf72d8 <col:30, line:8:1>
    |-DeclStmt 0xaf71e8 <line:4:3, col:19>
    | `-VarDecl 0xaf70a8 <col:3, col:18> col:7 used i 'int' cinit
    |   `-CallExpr 0xaf71c8 <col:11, col:18> 'int'
    |     `-ImplicitCastExpr 0xaf71b0 <col:11> 'int (*)()' <FunctionToPointerDecay>
    |       `-DeclRefExpr 0xaf7158 <col:11> 'int ()' lvalue Function 0xaf6eb8 'getInt' 'int ()'
    `-IfStmt 0xaf72c0 <line:6:3, line:7:5>
      |-CXXStaticCastExpr 0xaf7288 <line:6:7, col:30> 'bool' static_cast<_Bool> <NoOp>
      | `-BinaryOperator 0xaf7258 <col:25, col:29> 'bool' '<'
      |   |-ImplicitCastExpr 0xaf7240 <col:25> 'int' <LValueToRValue>
      |   | `-DeclRefExpr 0xaf7200 <col:25> 'int' lvalue Var 0xaf70a8 'i' 'int'
      |   `-IntegerLiteral 0xaf7220 <col:29> 'int' 7
      `-NullStmt 0xaf72b8 <line:7:5>
```
CFG:
```
void different_assignments()
 [B3 (ENTRY)]
   Succs (1): B2

 [B1]
   Preds (1): B2
   Succs (1): B0

 [B2]
   1: getInt
   2: [B2.1] (ImplicitCastExpr, FunctionToPointerDecay, int (*)(void))
   3: [B2.2]()
   4: int i = getInt();
   5: i
   6: [B2.5] (ImplicitCastExpr, LValueToRValue, int)
   7: 7
   8: [B2.6] < [B2.7]
   9: static_cast<bool>([B2.8]) (CXXStaticCastExpr, NoOp, _Bool)
   T: if [B2.9]
   Preds (1): B3
   Succs (2): B1 B0

 [B0 (EXIT)]
   Preds (2): B1 B2
```

I can see that a single statement has multiple CFGElements associated with it (implying that multiple checker callbacks might be invoked). Is your point tat we shouldn't do all the analysis at Block 2 CFGElement 6, but rather at Block 2 Element 9?

As I see this checker, what we want at the end of the day is to see a critical return value (say `ptr`) or **a derived value of it** (say `(ptr != nullptr && someOtherPrecondition()`) in a branch condition. But I think working from `checkBranchCondition` and working down the AST (does `ptr` or some other return value participate in this condition?)  , as opposed to using `checkLoation` and working working up (is `ptr`'s read here a part of a condition?) doesn't immediately feel like an easier, shorter or less error-prone solution.

A random idea, what we could try out is to store accesses to return values in `checkLocation` in the GDM, and check in `checkBranchCondition` whether its a substatement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72705