From 40adebeca0f99006d407508653c2cbd270a1a51c Mon Sep 17 00:00:00 2001 From: Richard Trieu Date: Mon, 5 Dec 2016 23:41:46 +0000 Subject: [PATCH] Warn on unsigned zero in call to std::max New default warning that triggers when an unsigned zero is used in a call to std::max. For unsigned values, zero is the minimum value, so any call to std::max is always equal to the other value. A common pattern was to take the max of zero and the difference of two unsigned values, not taking into account that unsigned values wrap around below zero. This warning also emits a note with a fixit hint to remove the zero and call to std::max. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@288732 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 1 + include/clang/Basic/DiagnosticSemaKinds.td | 8 ++ include/clang/Sema/Sema.h | 3 + lib/Sema/SemaChecking.cpp | 87 ++++++++++++++++++++++ 4 files changed, 99 insertions(+) diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index dc6ceefd15..e1e503737c 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -256,6 +256,7 @@ def LoopAnalysis : DiagGroup<"loop-analysis", [ForLoopAnalysis, def MalformedWarningCheck : DiagGroup<"malformed-warning-check">; def Main : DiagGroup<"main">; def MainReturnType : DiagGroup<"main-return-type">; +def MaxUnsignedZero : DiagGroup<"max-unsigned-zero">; def MissingBraces : DiagGroup<"missing-braces">; def MissingDeclarations: DiagGroup<"missing-declarations">; def : DiagGroup<"missing-format-attribute">; diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index bd91b67702..843329554d 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -53,6 +53,14 @@ def warn_pointer_abs : Warning< "taking the absolute value of %select{pointer|function|array}0 type %1 is suspicious">, InGroup; +def warn_max_unsigned_zero : Warning< + "taking the max of " + "%select{a value and unsigned zero|unsigned zero and a value}0 " + "is always equal to the other value">, + InGroup; +def note_remove_max_call : Note< + "remove call to max function and unsigned zero argument">; + def warn_infinite_recursive_function : Warning< "all paths through this function will call itself">, InGroup, DefaultIgnore; diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index 9531440c4f..fe65b08a08 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -9802,6 +9802,9 @@ private: const FunctionDecl *FDecl, IdentifierInfo *FnInfo); + void CheckMinZero(const CallExpr *Call, const FunctionDecl *FDecl, + IdentifierInfo *FnInfo); + void CheckMemaccessArguments(const CallExpr *Call, unsigned BId, IdentifierInfo *FnName); diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp index 9e16554c2f..bb68853f4e 100644 --- a/lib/Sema/SemaChecking.cpp +++ b/lib/Sema/SemaChecking.cpp @@ -2518,6 +2518,8 @@ bool Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall, return false; CheckAbsoluteValueFunction(TheCall, FDecl, FnInfo); + CheckMinZero(TheCall, FDecl, FnInfo); + if (getLangOpts().ObjC1) DiagnoseCStringFormatDirectiveInCFAPI(*this, FDecl, Args, NumArgs); @@ -6767,6 +6769,91 @@ void Sema::CheckAbsoluteValueFunction(const CallExpr *Call, Call->getCallee()->getSourceRange(), NewAbsKind, ArgType); } +//===--- CHECK: Warn on use of std::max and unsigned zero. r---------------===// +static bool IsFunctionStdMax(const FunctionDecl *FDecl) { + if (!FDecl) + return false; + + if (!FDecl->getIdentifier() || !FDecl->getIdentifier()->isStr("max")) + return false; + + const NamespaceDecl *ND = dyn_cast(FDecl->getDeclContext()); + + while (ND && ND->isInlineNamespace()) { + ND = dyn_cast(ND->getDeclContext()); + } + + if (!ND || !ND->getIdentifier() || !ND->getIdentifier()->isStr("std")) + return false; + + if (!isa(ND->getDeclContext())) + return false; + + return true; +} + +void Sema::CheckMinZero(const CallExpr *Call, const FunctionDecl *FDecl, + IdentifierInfo *FnInfo) { + if (!Call || !FDecl) return; + + // Ignore template specializations and macros. + if (!ActiveTemplateInstantiations.empty()) return; + if (Call->getExprLoc().isMacroID()) return; + + // Only care about the one template argument, two function parameter std::max + if (Call->getNumArgs() != 2) return; + if (!IsFunctionStdMax(FDecl)) return; + const auto * ArgList = FDecl->getTemplateSpecializationArgs(); + if (!ArgList) return; + if (ArgList->size() != 1) return; + + // Check that template type argument is unsigned integer. + const auto& TA = ArgList->get(0); + if (TA.getKind() != TemplateArgument::Type) return; + QualType ArgType = TA.getAsType(); + if (!ArgType->isUnsignedIntegerType()) return; + + // See if either argument is a literal zero. + auto IsLiteralZeroArg = [](const Expr* E) -> bool { + const auto *MTE = dyn_cast(E); + if (!MTE) return false; + const auto *Num = dyn_cast(MTE->GetTemporaryExpr()); + if (!Num) return false; + if (Num->getValue() != 0) return false; + return true; + }; + + const Expr *FirstArg = Call->getArg(0); + const Expr *SecondArg = Call->getArg(1); + const bool IsFirstArgZero = IsLiteralZeroArg(FirstArg); + const bool IsSecondArgZero = IsLiteralZeroArg(SecondArg); + + // Only warn when exactly one argument is zero. + if (IsFirstArgZero == IsSecondArgZero) return; + + SourceRange FirstRange = FirstArg->getSourceRange(); + SourceRange SecondRange = SecondArg->getSourceRange(); + + SourceRange ZeroRange = IsFirstArgZero ? FirstRange : SecondRange; + + Diag(Call->getExprLoc(), diag::warn_max_unsigned_zero) + << IsFirstArgZero << Call->getCallee()->getSourceRange() << ZeroRange; + + // Deduce what parts to remove so that "std::max(0u, foo)" becomes "(foo)". + SourceRange RemovalRange; + if (IsFirstArgZero) { + RemovalRange = SourceRange(FirstRange.getBegin(), + SecondRange.getBegin().getLocWithOffset(-1)); + } else { + RemovalRange = SourceRange(getLocForEndOfToken(FirstRange.getEnd()), + SecondRange.getEnd()); + } + + Diag(Call->getExprLoc(), diag::note_remove_max_call) + << FixItHint::CreateRemoval(Call->getCallee()->getSourceRange()) + << FixItHint::CreateRemoval(RemovalRange); +} + //===--- CHECK: Standard memory functions ---------------------------------===// /// \brief Takes the expression passed to the size_t parameter of functions -- 2.40.0