sorenj updated this revision to Diff 237466.
sorenj added a comment.
- Address documentation comments.
- Address documentation comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71607/new/
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/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
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/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst
===================================================================
--- /dev/null
+++ 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]); }
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ 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
+ <clang-tidy/checks/bugprone-unsiged-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
<clang-tidy/checks/cert-mem57-cpp>` check.
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 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
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
@@ -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<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
