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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

It looks great.
Let me check the test coverage and the generated docs page it everything looks great.



================
Comment at: clang/docs/analyzer/checkers.rst:2538
+may change value of ``errno`` if the call does not fail.
+Therefore ``errno`` should only be used if it is known from the return value
+of a function that the call has failed.
----------------



================
Comment at: clang/docs/analyzer/checkers.rst:2540
+of a function that the call has failed.
+There are exceptions to this rule but the affected functions are not
+yet supported by the checker.
----------------
Please mention at least one of those functions.


================
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",
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:145
+
+  if (IsLoad) {
+    switch (EState) {
----------------
Might be more readable to early return instead.


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:46
+
+  /// Indicates if a read (load) of 'errno' is allowed in a non-conditional
+  /// statement.
----------------
martong wrote:
> Or `in a statement without a condition` ?
I don't really understand this comment. What is a //non-condition part of a statement//?


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


================
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))
----------------
balazske wrote:
> 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.
I was thinking about this:
```lang=C++
bool isErrno(const Decl *D) {
  if (const auto *VD = dyn_cast_or_null<VarDecl>(D))
    if (const IdentifierInfo *II = VD->getIdentifier())
      return II->getName() == ErrnoVarName;
  if (const auto *FD = dyn_cast_or_null<FunctionDecl>(D))
    if (const IdentifierInfo *II = FD->getIdentifier())
      return llvm::is_contained(ErrnoLocationFuncNames, II->getName());
  return false;
}
```
This way both checks would follow the same pattern, symmetry.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:34
+  /// value.
+  Errno_MustNotBeChecked = 2
+};
----------------
balazske wrote:
> 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_`?
But you always refer to it by it's qualified name: `errno_modeling::Errno_Irrelevant`.
In which case this prefix is just noise IMO. That's what I'm about.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122150