[clang-tidy] Add check for implicit widening of multiplication result
Overflows are never fun.
In most cases (in most of the code), they are rare,
because usually you e.g. don't have as many elements.
However, it's exceptionally easy to fall into this pitfail
in code that deals with images, because, assuming 4-channel 32-bit FP data,
you need *just* ~269 megapixel image to case an overflow
when computing at least the total byte count.
In [[ https://github.com/darktable-org/darktable | darktable ]], there is a *long*, painful history of dealing with such bugs:
* https://github.com/darktable-org/darktable/pull/7740
* https://github.com/darktable-org/darktable/pull/7419
* https://github.com/darktable-org/darktable/commit/eea1989f2c9fa76710db07baaec4c19c1e40e81c
* https://github.com/darktable-org/darktable/commit/70626dd95bf0fab36f2d011dab075e3ebbf7aa28
* https://github.com/darktable-org/darktable/pull/670
* https://github.com/darktable-org/darktable/commit/38c69fb1b2bc90057c569242cb9945a10be0b583
and yet they clearly keep resurfacing still.
It would be immensely helpful to have a diagnostic for those patterns,
which is what this change proposes.
Currently, i only diagnose the most obvious case, where multiplication
is directly widened with no other expressions inbetween,
(i.e. `long r = (int)a * (int)b` but not even e.g. `long r = ((int)a * (int)b)`)
however that might be worth relaxing later.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D93822
2021-04-14 02:21:19 +08:00
|
|
|
//===--- ImplicitWideningOfMultiplicationResultCheck.cpp - clang-tidy -----===//
|
|
|
|
//
|
|
|
|
// 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 "ImplicitWideningOfMultiplicationResultCheck.h"
|
|
|
|
#include "clang/AST/ASTContext.h"
|
|
|
|
#include "clang/ASTMatchers/ASTMatchFinder.h"
|
|
|
|
|
|
|
|
using namespace clang::ast_matchers;
|
|
|
|
|
|
|
|
namespace clang {
|
|
|
|
namespace {
|
|
|
|
AST_MATCHER(ImplicitCastExpr, isPartOfExplicitCast) {
|
|
|
|
return Node.isPartOfExplicitCast();
|
|
|
|
}
|
|
|
|
} // namespace
|
|
|
|
} // namespace clang
|
|
|
|
|
|
|
|
namespace clang {
|
|
|
|
namespace tidy {
|
|
|
|
namespace bugprone {
|
|
|
|
|
|
|
|
static const Expr *getLHSOfMulBinOp(const Expr *E) {
|
|
|
|
assert(E == E->IgnoreParens() && "Already skipped all parens!");
|
|
|
|
// Is this: long r = int(x) * int(y); ?
|
|
|
|
// FIXME: shall we skip brackets/casts/etc?
|
|
|
|
const auto *BO = dyn_cast<BinaryOperator>(E);
|
|
|
|
if (!BO || BO->getOpcode() != BO_Mul)
|
|
|
|
// FIXME: what about: long r = int(x) + (int(y) * int(z)); ?
|
|
|
|
return nullptr;
|
|
|
|
return BO->getLHS()->IgnoreParens();
|
|
|
|
}
|
|
|
|
|
|
|
|
ImplicitWideningOfMultiplicationResultCheck::
|
|
|
|
ImplicitWideningOfMultiplicationResultCheck(StringRef Name,
|
|
|
|
ClangTidyContext *Context)
|
|
|
|
: ClangTidyCheck(Name, Context),
|
|
|
|
UseCXXStaticCastsInCppSources(
|
|
|
|
Options.get("UseCXXStaticCastsInCppSources", true)),
|
|
|
|
UseCXXHeadersInCppSources(Options.get("UseCXXHeadersInCppSources", true)),
|
|
|
|
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
|
[clang-tidy] Add a Standalone diagnostics mode to clang-tidy
Adds a flag to `ClangTidyContext` that is used to indicate to checks that fixes will only be applied one at a time.
This is to indicate to checks that each fix emitted should not depend on any other fixes emitted across the translation unit.
I've currently implemented the `IncludeInserter`, `LoopConvertCheck` and `PreferMemberInitializerCheck` to use these support these modes.
Reasoning behind this is in use cases like `clangd` it's only possible to apply one fix at a time.
For include inserter checks, the include is only added once for the first diagnostic that requires it, this will result in subsequent fixes not having the included needed.
A similar issue is seen in the `PreferMemberInitializerCheck` where the `:` will only be added for the first member that needs fixing.
Fixes emitted in `StandaloneDiagsMode` will likely result in malformed code if they are applied all together, conversely fixes currently emitted may result in malformed code if they are applied one at a time.
For this reason invoking `clang-tidy` from the binary will always with `StandaloneDiagsMode` disabled, However using it as a library its possible to select the mode you wish to use, `clangd` always selects `StandaloneDiagsMode`.
This is an example of the current behaviour failing
```lang=c++
struct Foo {
int A, B;
Foo(int D, int E) {
A = D;
B = E; // Fix Here
}
};
```
Incorrectly transformed to:
```lang=c++
struct Foo {
int A, B;
Foo(int D, int E), B(E) {
A = D;
// Fix Here
}
};
```
In `StandaloneDiagsMode`, it gets transformed to:
```lang=c++
struct Foo {
int A, B;
Foo(int D, int E) : B(E) {
A = D;
// Fix Here
}
};
```
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D97121
2022-04-16 16:53:32 +08:00
|
|
|
utils::IncludeSorter::IS_LLVM),
|
|
|
|
areDiagsSelfContained()) {}
|
[clang-tidy] Add check for implicit widening of multiplication result
Overflows are never fun.
In most cases (in most of the code), they are rare,
because usually you e.g. don't have as many elements.
However, it's exceptionally easy to fall into this pitfail
in code that deals with images, because, assuming 4-channel 32-bit FP data,
you need *just* ~269 megapixel image to case an overflow
when computing at least the total byte count.
In [[ https://github.com/darktable-org/darktable | darktable ]], there is a *long*, painful history of dealing with such bugs:
* https://github.com/darktable-org/darktable/pull/7740
* https://github.com/darktable-org/darktable/pull/7419
* https://github.com/darktable-org/darktable/commit/eea1989f2c9fa76710db07baaec4c19c1e40e81c
* https://github.com/darktable-org/darktable/commit/70626dd95bf0fab36f2d011dab075e3ebbf7aa28
* https://github.com/darktable-org/darktable/pull/670
* https://github.com/darktable-org/darktable/commit/38c69fb1b2bc90057c569242cb9945a10be0b583
and yet they clearly keep resurfacing still.
It would be immensely helpful to have a diagnostic for those patterns,
which is what this change proposes.
Currently, i only diagnose the most obvious case, where multiplication
is directly widened with no other expressions inbetween,
(i.e. `long r = (int)a * (int)b` but not even e.g. `long r = ((int)a * (int)b)`)
however that might be worth relaxing later.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D93822
2021-04-14 02:21:19 +08:00
|
|
|
|
|
|
|
void ImplicitWideningOfMultiplicationResultCheck::registerPPCallbacks(
|
|
|
|
const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
|
|
|
|
IncludeInserter.registerPreprocessor(PP);
|
|
|
|
}
|
|
|
|
|
|
|
|
void ImplicitWideningOfMultiplicationResultCheck::storeOptions(
|
|
|
|
ClangTidyOptions::OptionMap &Opts) {
|
|
|
|
Options.store(Opts, "UseCXXStaticCastsInCppSources",
|
|
|
|
UseCXXStaticCastsInCppSources);
|
|
|
|
Options.store(Opts, "UseCXXHeadersInCppSources", UseCXXHeadersInCppSources);
|
|
|
|
Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle());
|
|
|
|
}
|
|
|
|
|
|
|
|
llvm::Optional<FixItHint>
|
|
|
|
ImplicitWideningOfMultiplicationResultCheck::includeStddefHeader(
|
|
|
|
SourceLocation File) {
|
|
|
|
return IncludeInserter.createIncludeInsertion(
|
|
|
|
Result->SourceManager->getFileID(File),
|
|
|
|
ShouldUseCXXHeader ? "<cstddef>" : "<stddef.h>");
|
|
|
|
}
|
|
|
|
|
|
|
|
void ImplicitWideningOfMultiplicationResultCheck::handleImplicitCastExpr(
|
|
|
|
const ImplicitCastExpr *ICE) {
|
|
|
|
ASTContext *Context = Result->Context;
|
|
|
|
|
|
|
|
const Expr *E = ICE->getSubExpr()->IgnoreParens();
|
|
|
|
QualType Ty = ICE->getType();
|
|
|
|
QualType ETy = E->getType();
|
|
|
|
|
|
|
|
assert(!ETy->isDependentType() && !Ty->isDependentType() &&
|
|
|
|
"Don't expect to ever get here in template Context.");
|
|
|
|
|
|
|
|
// This must be a widening cast. Else we do not care.
|
|
|
|
unsigned SrcWidth = Context->getIntWidth(ETy);
|
|
|
|
unsigned TgtWidth = Context->getIntWidth(Ty);
|
|
|
|
if (TgtWidth <= SrcWidth)
|
|
|
|
return;
|
|
|
|
|
|
|
|
// Does the index expression look like it might be unintentionally computed
|
|
|
|
// in a narrower-than-wanted type?
|
|
|
|
const Expr *LHS = getLHSOfMulBinOp(E);
|
|
|
|
if (!LHS)
|
|
|
|
return;
|
|
|
|
|
|
|
|
// Ok, looks like we should diagnose this.
|
|
|
|
diag(E->getBeginLoc(), "performing an implicit widening conversion to type "
|
|
|
|
"%0 of a multiplication performed in type %1")
|
|
|
|
<< Ty << E->getType();
|
|
|
|
|
|
|
|
{
|
|
|
|
auto Diag = diag(E->getBeginLoc(),
|
|
|
|
"make conversion explicit to silence this warning",
|
|
|
|
DiagnosticIDs::Note)
|
|
|
|
<< E->getSourceRange();
|
|
|
|
|
|
|
|
if (ShouldUseCXXStaticCast)
|
|
|
|
Diag << FixItHint::CreateInsertion(
|
|
|
|
E->getBeginLoc(), "static_cast<" + Ty.getAsString() + ">(")
|
|
|
|
<< FixItHint::CreateInsertion(E->getEndLoc(), ")");
|
|
|
|
else
|
|
|
|
Diag << FixItHint::CreateInsertion(E->getBeginLoc(),
|
|
|
|
"(" + Ty.getAsString() + ")(")
|
|
|
|
<< FixItHint::CreateInsertion(E->getEndLoc(), ")");
|
|
|
|
Diag << includeStddefHeader(E->getBeginLoc());
|
|
|
|
}
|
|
|
|
|
|
|
|
QualType WideExprTy;
|
|
|
|
// Get Ty of the same signedness as ExprTy, because we only want to suggest
|
|
|
|
// to widen the computation, but not change it's signedness domain.
|
|
|
|
if (Ty->isSignedIntegerType() == ETy->isSignedIntegerType())
|
|
|
|
WideExprTy = Ty;
|
|
|
|
else if (Ty->isSignedIntegerType()) {
|
|
|
|
assert(ETy->isUnsignedIntegerType() &&
|
|
|
|
"Expected source type to be signed.");
|
|
|
|
WideExprTy = Context->getCorrespondingUnsignedType(Ty);
|
|
|
|
} else {
|
|
|
|
assert(Ty->isUnsignedIntegerType() &&
|
|
|
|
"Expected target type to be unsigned.");
|
|
|
|
assert(ETy->isSignedIntegerType() &&
|
|
|
|
"Expected source type to be unsigned.");
|
|
|
|
WideExprTy = Context->getCorrespondingSignedType(Ty);
|
|
|
|
}
|
|
|
|
|
|
|
|
{
|
|
|
|
auto Diag = diag(E->getBeginLoc(), "perform multiplication in a wider type",
|
|
|
|
DiagnosticIDs::Note)
|
|
|
|
<< LHS->getSourceRange();
|
|
|
|
|
|
|
|
if (ShouldUseCXXStaticCast)
|
|
|
|
Diag << FixItHint::CreateInsertion(LHS->getBeginLoc(),
|
|
|
|
"static_cast<" +
|
|
|
|
WideExprTy.getAsString() + ">(")
|
|
|
|
<< FixItHint::CreateInsertion(LHS->getEndLoc(), ")");
|
|
|
|
else
|
|
|
|
Diag << FixItHint::CreateInsertion(LHS->getBeginLoc(),
|
|
|
|
"(" + WideExprTy.getAsString() + ")");
|
|
|
|
Diag << includeStddefHeader(LHS->getBeginLoc());
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
void ImplicitWideningOfMultiplicationResultCheck::handlePointerOffsetting(
|
|
|
|
const Expr *E) {
|
|
|
|
ASTContext *Context = Result->Context;
|
|
|
|
|
|
|
|
// We are looking for a pointer offset operation,
|
|
|
|
// with one hand being a pointer, and another one being an offset.
|
|
|
|
const Expr *PointerExpr, *IndexExpr;
|
|
|
|
if (const auto *BO = dyn_cast<BinaryOperator>(E)) {
|
|
|
|
PointerExpr = BO->getLHS();
|
|
|
|
IndexExpr = BO->getRHS();
|
|
|
|
} else if (const auto *ASE = dyn_cast<ArraySubscriptExpr>(E)) {
|
|
|
|
PointerExpr = ASE->getLHS();
|
|
|
|
IndexExpr = ASE->getRHS();
|
|
|
|
} else
|
|
|
|
return;
|
|
|
|
|
|
|
|
if (IndexExpr->getType()->isPointerType())
|
|
|
|
std::swap(PointerExpr, IndexExpr);
|
|
|
|
|
|
|
|
if (!PointerExpr->getType()->isPointerType() ||
|
|
|
|
IndexExpr->getType()->isPointerType())
|
|
|
|
return;
|
|
|
|
|
|
|
|
IndexExpr = IndexExpr->IgnoreParens();
|
|
|
|
|
|
|
|
QualType IndexExprType = IndexExpr->getType();
|
|
|
|
|
|
|
|
// If the index expression's type is not known (i.e. we are in a template),
|
|
|
|
// we can't do anything here.
|
|
|
|
if (IndexExprType->isDependentType())
|
|
|
|
return;
|
|
|
|
|
|
|
|
QualType SSizeTy = Context->getPointerDiffType();
|
|
|
|
QualType USizeTy = Context->getSizeType();
|
|
|
|
QualType SizeTy = IndexExprType->isSignedIntegerType() ? SSizeTy : USizeTy;
|
|
|
|
// FIXME: is there a way to actually get the QualType for size_t/ptrdiff_t?
|
|
|
|
// Note that SizeTy.getAsString() will be unsigned long/..., NOT size_t!
|
|
|
|
StringRef TyAsString =
|
|
|
|
IndexExprType->isSignedIntegerType() ? "ptrdiff_t" : "size_t";
|
|
|
|
|
|
|
|
// So, is size_t actually wider than the result of the multiplication?
|
|
|
|
if (Context->getIntWidth(IndexExprType) >= Context->getIntWidth(SizeTy))
|
|
|
|
return;
|
|
|
|
|
|
|
|
// Does the index expression look like it might be unintentionally computed
|
|
|
|
// in a narrower-than-wanted type?
|
|
|
|
const Expr *LHS = getLHSOfMulBinOp(IndexExpr);
|
|
|
|
if (!LHS)
|
|
|
|
return;
|
|
|
|
|
|
|
|
// Ok, looks like we should diagnose this.
|
|
|
|
diag(E->getBeginLoc(),
|
|
|
|
"result of multiplication in type %0 is used as a pointer offset after "
|
|
|
|
"an implicit widening conversion to type '%1'")
|
|
|
|
<< IndexExprType << TyAsString;
|
|
|
|
|
|
|
|
{
|
|
|
|
auto Diag = diag(IndexExpr->getBeginLoc(),
|
|
|
|
"make conversion explicit to silence this warning",
|
|
|
|
DiagnosticIDs::Note)
|
|
|
|
<< IndexExpr->getSourceRange();
|
|
|
|
|
|
|
|
if (ShouldUseCXXStaticCast)
|
|
|
|
Diag << FixItHint::CreateInsertion(
|
|
|
|
IndexExpr->getBeginLoc(),
|
|
|
|
(Twine("static_cast<") + TyAsString + ">(").str())
|
|
|
|
<< FixItHint::CreateInsertion(IndexExpr->getEndLoc(), ")");
|
|
|
|
else
|
|
|
|
Diag << FixItHint::CreateInsertion(IndexExpr->getBeginLoc(),
|
|
|
|
(Twine("(") + TyAsString + ")(").str())
|
|
|
|
<< FixItHint::CreateInsertion(IndexExpr->getEndLoc(), ")");
|
|
|
|
Diag << includeStddefHeader(IndexExpr->getBeginLoc());
|
|
|
|
}
|
|
|
|
|
|
|
|
{
|
|
|
|
auto Diag =
|
|
|
|
diag(IndexExpr->getBeginLoc(), "perform multiplication in a wider type",
|
|
|
|
DiagnosticIDs::Note)
|
|
|
|
<< LHS->getSourceRange();
|
|
|
|
|
|
|
|
if (ShouldUseCXXStaticCast)
|
|
|
|
Diag << FixItHint::CreateInsertion(
|
|
|
|
LHS->getBeginLoc(),
|
|
|
|
(Twine("static_cast<") + TyAsString + ">(").str())
|
|
|
|
<< FixItHint::CreateInsertion(LHS->getEndLoc(), ")");
|
|
|
|
else
|
|
|
|
Diag << FixItHint::CreateInsertion(LHS->getBeginLoc(),
|
|
|
|
(Twine("(") + TyAsString + ")").str());
|
|
|
|
Diag << includeStddefHeader(LHS->getBeginLoc());
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
void ImplicitWideningOfMultiplicationResultCheck::registerMatchers(
|
|
|
|
MatchFinder *Finder) {
|
|
|
|
Finder->addMatcher(implicitCastExpr(unless(anyOf(isInTemplateInstantiation(),
|
|
|
|
isPartOfExplicitCast())),
|
|
|
|
hasCastKind(CK_IntegralCast))
|
|
|
|
.bind("x"),
|
|
|
|
this);
|
|
|
|
Finder->addMatcher(
|
|
|
|
arraySubscriptExpr(unless(isInTemplateInstantiation())).bind("x"), this);
|
|
|
|
Finder->addMatcher(binaryOperator(unless(isInTemplateInstantiation()),
|
|
|
|
hasType(isAnyPointer()),
|
|
|
|
hasAnyOperatorName("+", "-", "+=", "-="))
|
|
|
|
.bind("x"),
|
|
|
|
this);
|
|
|
|
}
|
|
|
|
|
|
|
|
void ImplicitWideningOfMultiplicationResultCheck::check(
|
|
|
|
const MatchFinder::MatchResult &Result) {
|
|
|
|
this->Result = &Result;
|
|
|
|
ShouldUseCXXStaticCast =
|
|
|
|
UseCXXStaticCastsInCppSources && Result.Context->getLangOpts().CPlusPlus;
|
|
|
|
ShouldUseCXXHeader =
|
|
|
|
UseCXXHeadersInCppSources && Result.Context->getLangOpts().CPlusPlus;
|
|
|
|
|
|
|
|
if (const auto *MatchedDecl = Result.Nodes.getNodeAs<ImplicitCastExpr>("x"))
|
|
|
|
handleImplicitCastExpr(MatchedDecl);
|
|
|
|
else if (const auto *MatchedDecl =
|
|
|
|
Result.Nodes.getNodeAs<ArraySubscriptExpr>("x"))
|
|
|
|
handlePointerOffsetting(MatchedDecl);
|
|
|
|
else if (const auto *MatchedDecl =
|
|
|
|
Result.Nodes.getNodeAs<BinaryOperator>("x"))
|
|
|
|
handlePointerOffsetting(MatchedDecl);
|
|
|
|
}
|
|
|
|
|
|
|
|
} // namespace bugprone
|
|
|
|
} // namespace tidy
|
|
|
|
} // namespace clang
|