[analyzer] Add new checker for unchecked return value.

[balazske]

Summary

Add a checker that checks if a function is used without examining
its return value. The set of functions is taken from CERT rule
ERR33-C "Detect and handle standard library errors". This is a
simplified check for the problem.

[balazske]

This is a very simplified form of the check that is implemented in D72705 but still may be useful in practical cases. This check could be a clang-tidy check but it is planned that the checker takes use of the planned new attributes to determine if a function should be checked.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp

42

Please note that the CallExpr does not necessarily stands alone. It may be wrapped into an ExprWithCleanUps. We should consider these CallExprs as unchecked too.

86

Hmm, why StringMap<>? Why not CallDescriptionMap<>?

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

11

Please use some valid format here. E.g. scanf("%*c");

17

Please add such simple test case for all the functions we try to check. (One call is enough for every such function, either in std:: or on the top namespace.)

[balazske]

Small fixes in test.

[balazske]

clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp

42

It looks like that the matcher finds these occurrences too. A test was added for it.

86

CallDescriptionMap is only usable with CallEvent that is not used in this checker. Or it can be extended with lookup from FunctionDecl?

[balazske]

Maybe it is better to have this check for every system function that has non-void return value (except specific functions)? This check is applicable to CERT rule ERR33-C, POS54-C and EXP12-C too (it is restricted but can find a part of the problems).

[balazske]

Ping

[Szelethus]

Looks like a neat checker! I guess the only question is the function matching, and I don't dislike it in its current state. @martong, do you have any thoughts on this?

On another note, have you checked this on some projects yet? I expect that we might need to tone down on some of the functions if this ends up being too noisy.

clang/lib/StaticAnalyzer/Checkers/UncheckedReturnValueChecker.cpp

66–67

I suppose we need this to compensate for the fact that we can't reliably match on standard C functions. Unfortunately, using CallDescription wouldn't necessarily solve this: D81745.

86

Good point. @martong, you have used FunctionDecl based checking, do you have any thoughts here?

[vsavchenko]

Great job!
I'd like to see more tests with the following cases:

• more expressions with function calls - ternary expression, arithmetic, array literals, initializer lists, etc.
• using/not using it in if statement with initializer
• using/not using it in different parts of GNU statement expressions
• calls in different parts of exception handling (both C++ and Obj-C)
• calls in lambdas/blocks/coroutines
• calls in different parts of C++ range-for loop
• calls in different parts of Obj-C for loop over collections
• calls under labels
• calls in attributed statements (e.g. [[clang::nomerge]] foo())
• calls in goto statements
• calls in Obj-C @synchronized and @autoreleasepool statements

[NoQ]

but it is planned that the checker takes use of the planned new attributes to determine if a function should be checked

This is literally __attribute__((warn_unused_result)) for which there already is a compiler warning.

[balazske]

There is a clang-tidy check bugprone-unused-return-value that may be confusing in relation to this checker. It does similar job for a selected set of functions and with different reason.