[PATCH] D122150: [clang][analyzer] Add checker for bad use of 'errno'.

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

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:553-554
+  CheckerOptions<[
+    CmdLineOption<Boolean,
+                  "AllowErrnoReadOutsideConditionExpressions",
+                  "Allow read of undefined value from errno outside of conditions",
----------------
steakhal wrote:
> Well, now I see why you marked this `Hide` previously.
> This is a //modeling// checker, thus it won't appear in the documentation nor in the `-analyzer-checker-help` etc.
> 
> Interestingly, other checkers mark some options `Hide` and other times `DontHide`.
> E.g. `DynamicMemoryModeling.Optimistic` is //displayed//, but the `DynamicMemoryModeling.AddNoOwnershipChangeNotes` is //hidden//.
> Whatever.
It looks good now, `ErrnoChecker` is not a modeling checker and the option should be accessible for users.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:47
+  /// Indicates if a read (load) of \c errno is allowed in a non-condition part
+  /// of \c if, \c switch, loop and conditional statements when the errno
+  /// value may be undefined.
----------------
steakhal wrote:
> You prefixed the previous two keywords with the `\c` marker. Why is it missing from the `switch`?
Is it really missing? (I do not check Doxygen documentation, probably the new documentation appears in the online pages after the commit.)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:100
+    case Expr::BinaryConditionalOperatorClass:
+      CondFound = (S == cast<BinaryConditionalOperator>(ParentS)->getCommon());
+      break;
----------------
steakhal wrote:
> Oh, so the test added for this actually uncovered a bug?
Yes: Even if it looks natural that `getCond` can be used the same way as at normal `ConditionalOperator` this is not true: It returns something other (casted or converted) than the root condition expressions.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:145
+
+  if (IsLoad) {
+    switch (EState) {
----------------
steakhal wrote:
> Might be more readable to early return instead.
There is a `else` part.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:205
+  if (CallF->isExternC() && CallF->isGlobal() &&
+      C.getSourceManager().isInSystemHeader(CallF->getLocation()) &&
+      !isErrno(CallF)) {
----------------
steakhal wrote:
> Does this refer to the callsite or to the Decl location?
> I think the former, which would be a bug I think.
`CallF` is a `FunctionDecl` that should be the original (first) declaration of the function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122150