llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Lucas Ly Ba (lucasly-ba) <details> <summary>Changes</summary> Comparing 'errno' against an integer literal is not portable because the values of the error constants are implementation-defined. Flag such comparisons and point to the 'E'-prefixed macros instead. Comparisons with 0 (the standard "no error" value) and comparisons whose literal comes from a macro such as errno == EINVAL are not flagged. Fixes #<!-- -->182518 --- Full diff: https://github.com/llvm/llvm-project/pull/210548.diff 8 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp (+3) - (modified) clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt (+1) - (added) clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.cpp (+56) - (added) clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.h (+32) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) - (added) clang-tools-extra/docs/clang-tidy/checks/bugprone/errno-comparison.rst (+19) - (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/errno-comparison.c (+23) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 3aa39d10ceb5d..a6b356aeac3a3 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -30,6 +30,7 @@ #include "DynamicStaticInitializersCheck.h" #include "EasilySwappableParametersCheck.h" #include "EmptyCatchCheck.h" +#include "ErrnoComparisonCheck.h" #include "ExceptionCopyConstructorThrowsCheck.h" #include "ExceptionEscapeCheck.h" #include "FloatLoopCounterCheck.h" @@ -165,6 +166,8 @@ class BugproneModule : public ClangTidyModule { CheckFactories.registerCheck<EasilySwappableParametersCheck>( "bugprone-easily-swappable-parameters"); CheckFactories.registerCheck<EmptyCatchCheck>("bugprone-empty-catch"); + CheckFactories.registerCheck<ErrnoComparisonCheck>( + "bugprone-errno-comparison"); CheckFactories.registerCheck<ExceptionCopyConstructorThrowsCheck>( "bugprone-exception-copy-constructor-throws"); CheckFactories.registerCheck<ExceptionEscapeCheck>( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 43e85b1407f21..c6933bc05fcd6 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -27,6 +27,7 @@ add_clang_library(clangTidyBugproneModule STATIC DynamicStaticInitializersCheck.cpp EasilySwappableParametersCheck.cpp EmptyCatchCheck.cpp + ErrnoComparisonCheck.cpp ExceptionCopyConstructorThrowsCheck.cpp ExceptionEscapeCheck.cpp FloatLoopCounterCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.cpp new file mode 100644 index 0000000000000..a8386f857c6e2 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.cpp @@ -0,0 +1,56 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ErrnoComparisonCheck.h" +#include "clang/AST/Expr.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +void ErrnoComparisonCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + binaryOperator( + isComparisonOperator(), + hasEitherOperand(ignoringParenImpCasts(integerLiteral().bind("lit")))) + .bind("cmp"), + this); +} + +void ErrnoComparisonCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Cmp = Result.Nodes.getNodeAs<BinaryOperator>("cmp"); + const auto *Lit = Result.Nodes.getNodeAs<IntegerLiteral>("lit"); + + // errno == 0 / errno != 0 is portable: 0 is the standard "no error" value. + if (Lit->getValue() == 0) + return; + + // A literal coming from a macro is the recommended form, e.g. errno == EINVAL. + if (Lit->getBeginLoc().isMacroID()) + return; + + // Pick the operand that is not the literal, keeping the casts/parens so its + // start location is the first token of the macro expansion. + const Expr *Other = Cmp->getLHS()->IgnoreParenImpCasts() == Lit + ? Cmp->getRHS() + : Cmp->getLHS(); + + // The other side must come from the 'errno' macro. + const SourceManager &SM = *Result.SourceManager; + SourceLocation Loc = Other->getBeginLoc(); + if (!Loc.isMacroID() || + Lexer::getImmediateMacroName(Loc, SM, getLangOpts()) != "errno") + return; + + diag(Cmp->getOperatorLoc(), + "comparing 'errno' against a literal is not portable"); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.h b/clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.h new file mode 100644 index 0000000000000..1041c26a2b39d --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ErrnoComparisonCheck.h @@ -0,0 +1,32 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ERRNOCOMPARISONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ERRNOCOMPARISONCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::bugprone { + +/// Flags comparisons of 'errno' against an integer literal. The values of the +/// error constants are implementation-defined, so a literal such as +/// 'errno == 5' is not portable; the 'E'-prefixed macros should be used. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/bugprone/errno-comparison.html +class ErrnoComparisonCheck : public ClangTidyCheck { +public: + ErrnoComparisonCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ERRNOCOMPARISONCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 69c3bcf67b8db..d900c1b8c1bdd 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -97,6 +97,12 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`bugprone-errno-comparison + <clang-tidy/checks/bugprone/errno-comparison>` check. + + Flags comparisons of ``errno`` against an integer literal, which is not + portable because the values of the error constants are implementation-defined. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/errno-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/errno-comparison.rst new file mode 100644 index 0000000000000..7155a31fc7b75 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/errno-comparison.rst @@ -0,0 +1,19 @@ +.. title:: clang-tidy - bugprone-errno-comparison + +bugprone-errno-comparison +========================= + +Flags comparisons of ``errno`` against an integer literal. + +The values of the ``errno`` error constants are implementation-defined, so +comparing ``errno`` against a hard-coded number such as ``errno == 5`` is not +portable. Use the ``E``-prefixed macros (e.g. ``EINVAL``) instead. + +.. code-block:: c + + if (errno == 5) {} // warning + if (errno == EINVAL) {} // ok, compared against the named macro + if (errno == 0) {} // ok, 0 is the standard "no error" value + +Comparisons with ``0`` and comparisons whose literal comes from a macro (such as +the ``E``-prefixed constants themselves) are not flagged. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 2a44dc78fbc89..73270750afb96 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -99,6 +99,7 @@ Clang-Tidy Checks :doc:`bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers>`, :doc:`bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters>`, :doc:`bugprone-empty-catch <bugprone/empty-catch>`, + :doc:`bugprone-errno-comparison <bugprone/errno-comparison>`, :doc:`bugprone-exception-copy-constructor-throws <bugprone/exception-copy-constructor-throws>`, :doc:`bugprone-exception-escape <bugprone/exception-escape>`, :doc:`bugprone-float-loop-counter <bugprone/float-loop-counter>`, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/errno-comparison.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/errno-comparison.c new file mode 100644 index 0000000000000..3d65358722755 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/errno-comparison.c @@ -0,0 +1,23 @@ +// RUN: %check_clang_tidy %s bugprone-errno-comparison %t + +extern int *__errno_location(void); +#define errno (*__errno_location()) +#define EINVAL 22 + +void positive(void) { + if (errno == 5) {} + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: comparing 'errno' against a literal is not portable [bugprone-errno-comparison] + if (errno != 5) {} + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: comparing 'errno' against a literal is not portable [bugprone-errno-comparison] + if (errno < 10) {} + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: comparing 'errno' against a literal is not portable [bugprone-errno-comparison] + if (5 == errno) {} + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: comparing 'errno' against a literal is not portable [bugprone-errno-comparison] +} + +void negative(int x) { + if (errno == 0) {} // errno == 0 is the portable "no error" check + if (errno != 0) {} + if (errno == EINVAL) {} // comparing against the named macro is the fix + if (x == 5) {} // not errno +} `````````` </details> https://github.com/llvm/llvm-project/pull/210548 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
