diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -54,6 +54,7 @@ #include "UndefinedMemoryManipulationCheck.h" #include "UndelegatedConstructorCheck.h" #include "UnhandledSelfAssignmentCheck.h" +#include "UnsignedSubtractionCheck.h" #include "UnusedRaiiCheck.h" #include "UnusedReturnValueCheck.h" #include "UseAfterMoveCheck.h" @@ -156,6 +157,8 @@ "bugprone-undelegated-constructor"); CheckFactories.registerCheck( "bugprone-unhandled-self-assignment"); + CheckFactories.registerCheck( + "bugprone-unsigned-subtraction"); CheckFactories.registerCheck( "bugprone-unused-raii"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h @@ -0,0 +1,38 @@ +//===--- UnsignedSubtractionCheck.h - clang-tidy ----------------*- C++ -*-===// +// +// 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_UNSIGNED_SUBTRACTION_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSIGNED_SUBTRACTION_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds cases where a signed value is subtracted from an unsigned value, +/// a likely cause of unexpected underflow. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unsigned-subtraction.html +class UnsignedSubtractionCheck : public ClangTidyCheck { + public: + UnsignedSubtractionCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + + private: + llvm::StringSet<> Visited; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSIGEND_SUBTRACTION_H diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp @@ -0,0 +1,139 @@ +//===--- UnsignedSubtractionCheck.cpp - clang-tidy --------------*- C++ -*-===// +// +// 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 "UnsignedSubtractionCheck.h" +#include "../utils/Matchers.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Tooling/FixIt.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) { + const auto UnsignedIntType = hasType(isUnsignedInteger()); + const auto SignedIntType = hasType(isSignedInteger()); + const auto SizeOf = anyOf(sizeOfExpr(has(type())), sizeOfExpr(has(expr()))); + const auto UnsignedSubtraction = binaryOperator( + hasOperatorName("-"), + hasLHS(expr(allOf(UnsignedIntType, unless(SizeOf))).bind("lhs")), + hasRHS(implicitCastExpr( + hasSourceExpression(anyOf(integerLiteral().bind("literal"), + expr(SignedIntType).bind("rhs")))))); + Finder->addMatcher(UnsignedSubtraction, this); + + // Track comparisons involving unsigned ints, to exclude cases where + // the code is (potentially) protected against underflows. + Finder->addMatcher(binaryOperator(matchers::isComparisonOperator(), + hasEitherOperand(UnsignedIntType)) + .bind("comparison"), + this); + + // Invocations of container.empty() will exempt checks for container.size(). + Finder->addMatcher( + callExpr(callee(functionDecl(hasName("empty"))), argumentCountIs(0)) + .bind("call-empty"), + this); +} + +void UnsignedSubtractionCheck::check(const MatchFinder::MatchResult &Result) { + const char *message = + "signed value subtracted from unsigned value is converted to unsigned; " + "with possible integer underflow. " + "When used in comparisons, consider moving term to other side as a sum " + "instead of a subtraction.\n"; + const SourceManager &SM = *Result.SourceManager; + const ASTContext &Ctx = *Result.Context; + + const auto *CallEmpty = Result.Nodes.getNodeAs("call-empty"); + if (CallEmpty) { + CharSourceRange SourceRange = + Lexer::makeFileCharRange(CharSourceRange::getTokenRange( + CallEmpty->getCallee()->getSourceRange()), + SM, Ctx.getLangOpts()); + if (SourceRange.isValid()) { + // Calls for foo.empty(), strip off the empty() and replace with size() + // to exempt calls to size() from being checked. + StringRef CalleeText = Lexer::getSourceText( + CharSourceRange::getTokenRange(CallEmpty->getSourceRange()), SM, + getLangOpts()); + Visited.insert( + CalleeText.str().substr(0, CalleeText.size() - 7 /* empty() */) + + "size()"); + } + return; + } + + const auto *Comparison = Result.Nodes.getNodeAs("comparison"); + if (Comparison) { + for (auto side : {Comparison->getLHS(), Comparison->getLHS()}) { + if (side->getType()->isUnsignedIntegerType()) { + std::string Text = tooling::fixit::getText(*side, Ctx).str(); + Visited.insert(Text); + } + } + return; + } + + const auto *lhs = Result.Nodes.getNodeAs("lhs"); + std::string Text = tooling::fixit::getText(*lhs, Ctx).str(); + if (Visited.count(Text)) { + // This expression has been seen in the context of a comparison, so + // presuming it is safe. + return; + } + + const auto *literal = Result.Nodes.getNodeAs("literal"); + const auto *rhs = literal ? literal : Result.Nodes.getNodeAs("rhs"); + + llvm::APSInt leftConstant; + llvm::APSInt rightConstant; + if (lhs->isIntegerConstantExpr(leftConstant, Ctx) && + rhs->isIntegerConstantExpr(rightConstant, Ctx)) { + // If both values are compile time constants, do not warn. + if (llvm::APSInt::compareValues(leftConstant, rightConstant) > 0) return; + } + + if (literal) { + CharSourceRange SourceRange = Lexer::makeFileCharRange( + CharSourceRange::getTokenRange(rhs->getSourceRange()), SM, + Ctx.getLangOpts()); + if (SourceRange.isValid()) { + StringRef NumberText = Lexer::getSourceText( + CharSourceRange::getTokenRange(rhs->getSourceRange()), SM, + getLangOpts()); + if (NumberText.find_first_not_of("0123456789") == StringRef::npos) { + diag(rhs->getBeginLoc(), message) + << FixItHint::CreateInsertion(SourceRange.getEnd(), "u"); + return; + } + } + } + // Also handles fall through cases where the literal couldn't be fixed with + // a suffix. + CharSourceRange SourceRange = Lexer::makeFileCharRange( + CharSourceRange::getTokenRange(rhs->getSourceRange()), SM, + Ctx.getLangOpts()); + DiagnosticBuilder Diag = diag(rhs->getBeginLoc(), message); + if (SourceRange.isInvalid()) { + // An invalid source range likely means we are inside a macro body. A manual + // fix is likely needed so we do not create a fix-it hint. + return; + } + Diag << FixItHint::CreateInsertion( + SourceRange.getBegin(), + "static_castgetType().getAsString() + ">(") + << FixItHint::CreateInsertion(SourceRange.getEnd(), ")"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -100,6 +100,12 @@ Finds ``signed char`` to integer conversions which might indicate a programming error. +- New :doc:`bugprone-unsigned-subtraction + ` check. + + Finds expresssions where a signed value is subtracted from an + unsigned value, a likely cause of unexpected underflow. + - New :doc:`cert-mem57-cpp ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst @@ -0,0 +1,34 @@ +.. title:: clang-tidy - bugprone-unsigned-subtraction + +bugprone-sizeof-expression +========================== + +Finds expressions where a signed value is subtracted from an +unsigned value, a likely cause of unexpected underflow. + +Many programmers are unaware that an expression of unsigned - signed +will promote the signed argument to unsigned, and if an underflow +occurs it results in a large positive value. Hence the frequent +errors related to to tests of ``container.size() - 1 <= 0`` when a +container is empty. + +This check suggest a fix-it change that will append a ``u`` to +constants, thus making the implict cast explicit and signals that +the the code was intended. In cases where the second argument is +not a constant, a ``static_cast`` is recommended. Heuristics are +employed to reduce warnings in contexts where the subtraction +may be safe. + +In many cases, the subtraction can be avoided entirely, consider: + +.. code-block:: c++ + + if (x <= container.size() - 1) { func(container[x]); } + +moving the subtraction to the other side avoids the potential +underflow + +.. code-block:: c++ + + if (x + 1 <= container.size()) { func(container[x]); } + diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp @@ -0,0 +1,104 @@ +// RUN: %check_clang_tidy %s bugprone-unsigned-subtraction %t + +template +class vector { + public: + unsigned size(); + bool empty(); +}; + +#define MACRO_MINUS(x) x - 5 +#define MACRO_INT 20 + +void signedSubtracted() { + unsigned x = 5; + int yyyy = 2; + if (x - yyyy == 0) { + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from + // CHECK-FIXES: if (x - static_cast(yyyy) == 0) { + return; + } + if (0 >= x - yyyy) { + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from + // CHECK-FIXES: if (0 >= x - static_cast(yyyy)) { + return; + } + unsigned z = MACRO_MINUS(x); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from + z = x - MACRO_INT; + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from + // CHECK-FIXES: z = x - static_cast(MACRO_INT); +} + +void signedConstantSubtracted() { + unsigned x = 5; + if (x - 2 > 0) { + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from + // CHECK-FIXES: if (x - 2u > 0) { + return; + } +} + +void casesThatShouldBeIgnored() { + unsigned x = 5; + // If the constant used in subtraction is already explicitly unsigned, do not + // warn. This is not safe, but the user presumably knows what they are doing. + if (x - 2u > 0u) { + return; + } + if (x - 2u > 0) { + return; + } + // sizeof operators are strictly positive for all types, and a constexpr, so + // any underflow would happen at compile time, so do not warn. + x = sizeof(long double) - 1; + // If both sides of the subtraction are compile time constants, don't warn. + if (5u - 2 > 0) { + return; + } + constexpr long y = 4; // NOLINT(runtime/int) + if (y - 4 > 0) { + return; + } +} + +// If the first argument of the subtraction is an expression that was previously +// used in a comparison, the user is presumed to have done something smart. +// This isn't perfect, but it greatly reduces false alarms. +void contextMatters() { + unsigned x = 5; + if (x < 5) return; + if (x - 2 > 0u) { + return; + } +} + +// For loops with a compartor meet the previously described case, and therefore +// do not warn if the variable is used in a subtraction. Again not perfect, but +// this code is complex to reason about and it's best not to generate false +// alarms. +unsigned forLoops() { + unsigned x; + for (unsigned i = 1; i < 5; ++i) { + x += i - 1; + } + return x; +} + +// Testing for an empty container before subtracting from size() is considered +// to be the same as comparing against the size - it's still possible to fail +// but reduces the number of false positives. +void containersEmpty() { + vector x; + if (!x.empty()) { + if (x.size() - 1 > 0) { + return; + } + } + vector y; + if (y.size() - 1 > 0) { + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from + // CHECK-FIXES: if (y.size() - 1u > 0) { + return; + } +}