[PATCH] D71510: [clang][checkers] Added new checker 'error-return-checker'. (WIP)

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:77
+// This should work with any type of null value.
+// FIXME: Is this different from the ValueErrorResultChecker with 0 as value?
+class NullErrorResultChecker : public CheckForErrorResultChecker {
----------------
gamesh411 wrote:
> I think the current implementation is slightly different. The ValueErrorResultChecker uses symbolic evaluation of the equation with a ConcreteInt 0 value, while the ProgramState::isNull checks if the value is a constant and if it is not a zero constant (also calls further to ConstraintManager::isNull).
I think if there is a method for a special purpose (`isNull`) it is better to use that, it can be more efficient.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:158
+// value < 0" check too.
+class NegativeOrEofErrorResultChecker : public CheckForErrorResultChecker {
+public:
----------------
gamesh411 wrote:
> Maybe only checking for negative value is enough? My gut feeling is that the equality check is redundant. I would argue:
> let A be: Value is negative
> let B be: Value is equal to -1
> 
> since B implies A, (A or B) can be simplified to just A.
> This presupposes that every time B can reasoned about A can be too.
> This seems to be the case for here as well, as the definiteness of being equal to -1 is stronger than being lower than 0.
> 
The test
```
void test_NegativeOrEofCorrectCheck1() {
  int X = fputs("", NULL);
  if (X == -1) {
  }
}
```
fails when only the check for negative value is there.

The problem is with the negated part: If there is a "X==-1" statement in the code to check then the "X<0" assumption is true but "X>=0" is not known because X can be less than -1. This check should accept the "X==-1" and "X<0" type of code.


================
Comment at: clang/test/Analysis/error-return.c:270
+
+These are OK if not checked:
+
----------------
gamesh411 wrote:
> just for the sake of completeness; mention all the cases where error checking is deemed unnecessary as per ERR-33-EX1
> 
> https://wiki.sei.cmu.edu/confluence/display/c/ERR33-C.+Detect+and+handle+standard+library+errors#ERR33-C.Detectandhandlestandardlibraryerrors-Exceptions
It seems that the better solution is to check for those functions too, and add a feature to the checker that ignores functions that are casted to `void`. The exception list says that even in these exceptions the `(void)` cast is needed. (Probably a other kind of warning message can be used for these functions.) The "function can not fail" and "return value is inconsequential" cases are not detectable by the checker (these cases depend on the context of the function call, not on the function itself, otherwise the function should appear in the list of functions to be ignored).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71510