steakhal added a comment. I believe `evalAssume` would be a better fit for diagnosing such issues, but I can see your point that we don't have `CheckerContext` there to emit reports. That being said, the `check::Location` is the best alternative.
Please also check the not `done` comments. ================ Comment at: clang/docs/analyzer/checkers.rst:2529 +Check for unsafe use of ``errno``. +This checker can find the first read of ``errno`` after not failed standard +function calls. ---------------- successful ================ 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. + ---------------- 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. ================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:550 +def ErrnoChecker : Checker<"Errno">, + HelpText<"Check not recommended uses of 'errno'">, + Dependencies<[ErrnoModeling]>, ---------------- antipatterns? ================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:554 + CmdLineOption<Boolean, + "AllowUnconditionalErrnoRead", + "Allow read of undefined value from errno outside of conditions", ---------------- My problem with this name is that `Unconditional` seems to bind to the action, not to the place where the read happens (inside condition expressions). I find this pretty confusing. How about calling this `AllowReadingErrnoInsideConditionExpressions`. Still bad, but probably better. ================ Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:558 + InAlpha, + Hide>, // ?? + ]>, ---------------- balazske wrote: > steakhal wrote: > > I think we have 'Hidden', but not 'Hide'. > > It means that it is a modeling checker. Any checker that emits diagnostics, > > should **not** be marked 'Hidden'. > This "Hide" applies for the option, not for the checker. It was copied from > another place. I am not sure if it must be used here. Quote `CheckerBase.td:39` ``` /// Marks the entry hidden. Hidden entries won't be displayed in /// -analyzer-checker-option-help. class HiddenEnum<bit val> { bit Val = val; } def DontHide : HiddenEnum<0>; def Hide : HiddenEnum<1>; ``` I believe, we should expose this option. Otherwise why would we introduce it in the first place? The default value of this is actually `DontHide` in the `CmdLineOption<>` td class. So, you should be fine by simply omitting this keyword here. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:10-11 +// This defines an "errno checker" that can detect some invalid use of the +// system-defined value 'errno'. This checker works together with the +// ErrnoModeling checker. +// ---------------- and the StdLibraryFunctionsChecker per D125400. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:101-102 + break; + default: + break; + } ---------------- 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. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:261 + if (unsigned EState = State->get<ErrnoState>()) + return static_cast<ErrnoCheckState>(EState); + return Errno_Irrelevant; ---------------- 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. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.cpp:280 + BR.markNotInteresting(ErrnoR); + return Message.c_str(); + } ---------------- Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122150/new/ https://reviews.llvm.org/D122150 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits