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

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

balazske added inline comments.


================
Comment at: clang/docs/analyzer/checkers.rst:2538-2539
+yet supported by the checker.
+The return values for the failure cases are documented in the standard Linux man
+pages of the functions.
+
----------------
steakhal wrote:
> Maybe place a crossref for this.
> 
> Also, am I right that there is a CERT rule about this issue?
> We should probably mention that, while placing a crossref too.
Probably there is not a preferred or "standard" site for man page lookup and it can be done from command line too. But I can include a  POSIX link https://pubs.opengroup.org/onlinepubs/9699919799/.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:550
+def ErrnoChecker : Checker<"Errno">,
+  HelpText<"Check not recommended uses of 'errno'">,
+  Dependencies<[ErrnoModeling]>,
----------------
steakhal wrote:
> antipatterns?
I like "improper use of `errno`" better, it is similar to `HelpText` of other checkers.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:101-102
+      break;
+    default:
+      break;
+    }
----------------
steakhal wrote:
> We should also handle `BinaryOperator` for conditional operators (EQ, NE, LT, LE, ...). Or at least leave a FIXME about this.
> Have a test demonstrating the behavior.
The idea is here that this search does not stop at binary operators but goes up the AST until a condition is found.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:261
+  if (unsigned EState = State->get<ErrnoState>())
+    return static_cast<ErrnoCheckState>(EState);
+  return Errno_Irrelevant;
----------------
steakhal wrote:
> I believe you no longer need to cast enums to numbers and back for enums and other types.
> `get<ErrnoState>()` should return the right type.
> See D126801.
How to handle if no value exists?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:280
+      BR.markNotInteresting(ErrnoR);
+      return Message.c_str();
+    }
----------------
steakhal wrote:
> 
I can not omit `c_str`, there is a compile error probably related to  lambda return type.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:267-268
+  if (const auto *VD = dyn_cast_or_null<VarDecl>(D))
+    return VD->getIdentifier() &&
+           VD->getIdentifier()->getName() == ErrnoVarName;
+  if (const auto *FD = dyn_cast_or_null<FunctionDecl>(D))
----------------
steakhal wrote:
> It looks odd that in the very next `if` block you have `II` and here you don't.
I do not get what you mean here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:34
+  /// value.
+  Errno_MustNotBeChecked = 2
+};
----------------
steakhal wrote:
> You don't need to prefix these with `Errno_`. It's already contained by a specific namespace.
> https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> > Unless the enumerators are defined in their own small namespace or inside a class, enumerators should have a prefix corresponding to the enum declaration name
Without `Errno_` these sound too generic. Probably have only `Errno` prefix, not `Errno_`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122150