sorenj created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Many C++ programmers are unaware that an expression of unsigned - signed will
promote the signed argument to unsigned, and the resulting underflow produces
a large positive rather than negative result. Hence the frequent errors
related to the test x.size() - 1 <= 0 when the container x is empty.
This clang tidy detects signed values being subtracted from unsigned values
and warns the user about the potential error. It is not perfect as it is
not always possible at compile time to reason about code when this comparison
is made.
The warning also suggests a fixit change that will append a "u" to numerical
constants - this makes the implicit cast explicit and signals that the
developer knew what they were doing in a subtraction. In other cases it
suggests the rather abhorrent static_cast<>().
The easiest fix is to not do subtraction at all, just move the operation
to the other side of the comparison where it becomes an addition - which
has none of these surprising properties.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D71607
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp
clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp
===================================================================
--- /dev/null
+++ 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 T>
+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<unsigned int>(yyyy) == 0) {
+ return;
+ }
+ if (0 >= x - yyyy) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+ // CHECK-FIXES: if (0 >= x - static_cast<unsigned int>(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<unsigned int>(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<int> x;
+ if (!x.empty()) {
+ if (x.size() - 1 > 0) {
+ return;
+ }
+ }
+ vector<double> y;
+ if (y.size() - 1 > 0) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from
+ // CHECK-FIXES: if (y.size() - 1u > 0) {
+ return;
+ }
+}
Index: clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h
===================================================================
--- /dev/null
+++ 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 substracted 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
Index: clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp
===================================================================
--- /dev/null
+++ 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<CallExpr>("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<BinaryOperator>("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<Expr>("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<IntegerLiteral>("literal");
+ const auto *rhs = literal ? literal : Result.Nodes.getNodeAs<Expr>("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_cast<unsigned " + rhs->getType().getAsString() + ">(")
+ << FixItHint::CreateInsertion(SourceRange.getEnd(), ")");
+}
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -53,6 +53,7 @@
#include "UndefinedMemoryManipulationCheck.h"
#include "UndelegatedConstructorCheck.h"
#include "UnhandledSelfAssignmentCheck.h"
+#include "UnsignedSubtractionCheck.h"
#include "UnusedRaiiCheck.h"
#include "UnusedReturnValueCheck.h"
#include "UseAfterMoveCheck.h"
@@ -153,6 +154,8 @@
"bugprone-undelegated-constructor");
CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
"bugprone-unhandled-self-assignment");
+ CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
+ "bugprone-unsigned-subtraction");
CheckFactories.registerCheck<UnusedRaiiCheck>(
"bugprone-unused-raii");
CheckFactories.registerCheck<UnusedReturnValueCheck>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
