Skip to content
Permalink
Browse files

Merge and improve code that detects same value in comparisons.

-Wtautological-overlap-compare and self-comparison from -Wtautological-compare
relay on detecting the same operand in different locations.  Previously, each
warning had it's own operand checker.  Now, both are merged together into
one function that each can call.  The function also now looks through member
access and array accesses.

Differential Revision: https://reviews.llvm.org/D66045

llvm-svn: 372453
  • Loading branch information...
Richard Trieu
Richard Trieu committed Sep 21, 2019
1 parent 27a8039 commit 4c05de8c1d157f4b5e0091a52dbbcce7242ee485
@@ -53,6 +53,9 @@ Improvements to Clang's diagnostics

- -Wtautological-overlap-compare will warn on negative numbers and non-int
types.
- -Wtautological-compare for self comparisons and
-Wtautological-overlap-compare will now look through member and array
access to determine if two operand expressions are the same.

Non-comprehensive list of changes in this release
-------------------------------------------------
@@ -906,6 +906,11 @@ class Expr : public ValueStmt {
return skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
}

/// Checks that the two Expr's will refer to the same value as a comparison
/// operand. The caller must ensure that the values referenced by the Expr's
/// are not modified between E1 and E2 or the result my be invalid.
static bool isSameComparisonOperand(const Expr* E1, const Expr* E2);

static bool classof(const Stmt *T) {
return T->getStmtClass() >= firstExprConstant &&
T->getStmtClass() <= lastExprConstant;
@@ -3921,6 +3921,111 @@ bool Expr::refersToGlobalRegisterVar() const {
return false;
}

bool Expr::isSameComparisonOperand(const Expr* E1, const Expr* E2) {
E1 = E1->IgnoreParens();
E2 = E2->IgnoreParens();

if (E1->getStmtClass() != E2->getStmtClass())
return false;

switch (E1->getStmtClass()) {
default:
return false;
case CXXThisExprClass:
return true;
case DeclRefExprClass: {
// DeclRefExpr without an ImplicitCastExpr can happen for integral
// template parameters.
const auto *DRE1 = cast<DeclRefExpr>(E1);
const auto *DRE2 = cast<DeclRefExpr>(E2);
return DRE1->isRValue() && DRE2->isRValue() &&
DRE1->getDecl() == DRE2->getDecl();
}
case ImplicitCastExprClass: {
// Peel off implicit casts.
while (true) {
const auto *ICE1 = dyn_cast<ImplicitCastExpr>(E1);
const auto *ICE2 = dyn_cast<ImplicitCastExpr>(E2);
if (!ICE1 || !ICE2)
return false;
if (ICE1->getCastKind() != ICE2->getCastKind())
return false;
E1 = ICE1->getSubExpr()->IgnoreParens();
E2 = ICE2->getSubExpr()->IgnoreParens();
// The final cast must be one of these types.
if (ICE1->getCastKind() == CK_LValueToRValue ||
ICE1->getCastKind() == CK_ArrayToPointerDecay ||
ICE1->getCastKind() == CK_FunctionToPointerDecay) {
break;
}
}

const auto *DRE1 = dyn_cast<DeclRefExpr>(E1);
const auto *DRE2 = dyn_cast<DeclRefExpr>(E2);
if (DRE1 && DRE2)
return declaresSameEntity(DRE1->getDecl(), DRE2->getDecl());

const auto *Ivar1 = dyn_cast<ObjCIvarRefExpr>(E1);
const auto *Ivar2 = dyn_cast<ObjCIvarRefExpr>(E2);
if (Ivar1 && Ivar2) {
return Ivar1->isFreeIvar() && Ivar2->isFreeIvar() &&
declaresSameEntity(Ivar1->getDecl(), Ivar2->getDecl());
}

const auto *Array1 = dyn_cast<ArraySubscriptExpr>(E1);
const auto *Array2 = dyn_cast<ArraySubscriptExpr>(E2);
if (Array1 && Array2) {
if (!isSameComparisonOperand(Array1->getBase(), Array2->getBase()))
return false;

auto Idx1 = Array1->getIdx();
auto Idx2 = Array2->getIdx();
const auto Integer1 = dyn_cast<IntegerLiteral>(Idx1);
const auto Integer2 = dyn_cast<IntegerLiteral>(Idx2);
if (Integer1 && Integer2) {
if (Integer1->getValue() != Integer2->getValue())
return false;
} else {
if (!isSameComparisonOperand(Idx1, Idx2))
return false;
}

return true;
}

// Walk the MemberExpr chain.
while (isa<MemberExpr>(E1) && isa<MemberExpr>(E2)) {
const auto *ME1 = cast<MemberExpr>(E1);
const auto *ME2 = cast<MemberExpr>(E2);
if (!declaresSameEntity(ME1->getMemberDecl(), ME2->getMemberDecl()))
return false;
if (const auto *D = dyn_cast<VarDecl>(ME1->getMemberDecl()))
if (D->isStaticDataMember())
return true;
E1 = ME1->getBase()->IgnoreParenImpCasts();
E2 = ME2->getBase()->IgnoreParenImpCasts();
}

if (isa<CXXThisExpr>(E1) && isa<CXXThisExpr>(E2))
return true;

// A static member variable can end the MemberExpr chain with either
// a MemberExpr or a DeclRefExpr.
auto getAnyDecl = [](const Expr *E) -> const ValueDecl * {
if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
return DRE->getDecl();
if (const auto *ME = dyn_cast<MemberExpr>(E))
return ME->getMemberDecl();
return nullptr;
};

const ValueDecl *VD1 = getAnyDecl(E1);
const ValueDecl *VD2 = getAnyDecl(E2);
return declaresSameEntity(VD1, VD2);
}
}
}

/// isArrow - Return true if the base expression is a pointer to vector,
/// return false if the base expression is a vector.
bool ExtVectorElementExpr::isArrow() const {
@@ -105,12 +105,12 @@ static const Expr *tryTransformToIntOrEnumConstant(const Expr *E) {
return nullptr;
}

/// Tries to interpret a binary operator into `Decl Op Expr` form, if Expr is
/// an integer literal or an enum constant.
/// Tries to interpret a binary operator into `Expr Op NumExpr` form, if
/// NumExpr is an integer literal or an enum constant.
///
/// If this fails, at least one of the returned DeclRefExpr or Expr will be
/// null.
static std::tuple<const DeclRefExpr *, BinaryOperatorKind, const Expr *>
static std::tuple<const Expr *, BinaryOperatorKind, const Expr *>
tryNormalizeBinaryOperator(const BinaryOperator *B) {
BinaryOperatorKind Op = B->getOpcode();

@@ -132,8 +132,7 @@ tryNormalizeBinaryOperator(const BinaryOperator *B) {
Constant = tryTransformToIntOrEnumConstant(B->getLHS());
}

auto *D = dyn_cast<DeclRefExpr>(MaybeDecl->IgnoreParenImpCasts());
return std::make_tuple(D, Op, Constant);
return std::make_tuple(MaybeDecl, Op, Constant);
}

/// For an expression `x == Foo && x == Bar`, this determines whether the
@@ -1045,34 +1044,34 @@ class CFGBuilder {
if (!LHS->isComparisonOp() || !RHS->isComparisonOp())
return {};

const DeclRefExpr *Decl1;
const Expr *Expr1;
const Expr *DeclExpr1;
const Expr *NumExpr1;
BinaryOperatorKind BO1;
std::tie(Decl1, BO1, Expr1) = tryNormalizeBinaryOperator(LHS);
std::tie(DeclExpr1, BO1, NumExpr1) = tryNormalizeBinaryOperator(LHS);

if (!Decl1 || !Expr1)
if (!DeclExpr1 || !NumExpr1)
return {};

const DeclRefExpr *Decl2;
const Expr *Expr2;
const Expr *DeclExpr2;
const Expr *NumExpr2;
BinaryOperatorKind BO2;
std::tie(Decl2, BO2, Expr2) = tryNormalizeBinaryOperator(RHS);
std::tie(DeclExpr2, BO2, NumExpr2) = tryNormalizeBinaryOperator(RHS);

if (!Decl2 || !Expr2)
if (!DeclExpr2 || !NumExpr2)
return {};

// Check that it is the same variable on both sides.
if (Decl1->getDecl() != Decl2->getDecl())
if (!Expr::isSameComparisonOperand(DeclExpr1, DeclExpr2))
return {};

// Make sure the user's intent is clear (e.g. they're comparing against two
// int literals, or two things from the same enum)
if (!areExprTypesCompatible(Expr1, Expr2))
if (!areExprTypesCompatible(NumExpr1, NumExpr2))
return {};

Expr::EvalResult L1Result, L2Result;
if (!Expr1->EvaluateAsInt(L1Result, *Context) ||
!Expr2->EvaluateAsInt(L2Result, *Context))
if (!NumExpr1->EvaluateAsInt(L1Result, *Context) ||
!NumExpr2->EvaluateAsInt(L2Result, *Context))
return {};

llvm::APSInt L1 = L1Result.Val.getInt();
@@ -10245,20 +10245,18 @@ static void diagnoseLogicalNotOnLHSofCheck(Sema &S, ExprResult &LHS,
<< FixItHint::CreateInsertion(SecondClose, ")");
}

// Get the decl for a simple expression: a reference to a variable,
// an implicit C++ field reference, or an implicit ObjC ivar reference.
static ValueDecl *getCompareDecl(Expr *E) {
if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E))
return DR->getDecl();
if (ObjCIvarRefExpr *Ivar = dyn_cast<ObjCIvarRefExpr>(E)) {
if (Ivar->isFreeIvar())
return Ivar->getDecl();
}
if (MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
// Returns true if E refers to a non-weak array.
static bool checkForArray(const Expr *E) {
const ValueDecl *D = nullptr;
if (const DeclRefExpr *DR = dyn_cast<DeclRefExpr>(E)) {
D = DR->getDecl();
} else if (const MemberExpr *Mem = dyn_cast<MemberExpr>(E)) {
if (Mem->isImplicitAccess())
return Mem->getMemberDecl();
D = Mem->getMemberDecl();
}
return nullptr;
if (!D)
return false;
return D->getType()->isArrayType() && !D->isWeak();
}

/// Diagnose some forms of syntactically-obvious tautological comparison.
@@ -10291,8 +10289,6 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
// obvious cases in the definition of the template anyways. The idea is to
// warn when the typed comparison operator will always evaluate to the same
// result.
ValueDecl *DL = getCompareDecl(LHSStripped);
ValueDecl *DR = getCompareDecl(RHSStripped);

// Used for indexing into %select in warn_comparison_always
enum {
@@ -10301,7 +10297,8 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
AlwaysFalse,
AlwaysEqual, // std::strong_ordering::equal from operator<=>
};
if (DL && DR && declaresSameEntity(DL, DR)) {

if (Expr::isSameComparisonOperand(LHS, RHS)) {
unsigned Result;
switch (Opc) {
case BO_EQ: case BO_LE: case BO_GE:
@@ -10321,9 +10318,7 @@ static void diagnoseTautologicalComparison(Sema &S, SourceLocation Loc,
S.PDiag(diag::warn_comparison_always)
<< 0 /*self-comparison*/
<< Result);
} else if (DL && DR &&
DL->getType()->isArrayType() && DR->getType()->isArrayType() &&
!DL->isWeak() && !DR->isWeak()) {
} else if (checkForArray(LHSStripped) && checkForArray(RHSStripped)) {
// What is it always going to evaluate to?
unsigned Result;
switch(Opc) {
@@ -1,20 +1,26 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
// RUN: -Wno-tautological-compare\
// RUN: -x c %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
// RUN: -Wno-tautological-compare\
// RUN: -x c++ -std=c++14 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
// RUN: -Wno-tautological-compare\
// RUN: -x c++ -std=c++17 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c++ -std=c++14 %s
// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
// RUN: -analyzer-checker=debug.ExprInspection -verify\
// RUN: -Wno-tautological-compare\
// RUN: -DINLINE -x c++ -std=c++17 %s

void clang_analyzer_eval(int);
@@ -158,3 +158,17 @@ int no_warning(unsigned x) {
return x >= 0 || x == 1;
// no warning since "x >= 0" is caught by a different tautological warning.
}

struct A {
int x;
int y;
};

int struct_test(struct A a) {
return a.x > 5 && a.y < 1; // no warning, different variables

return a.x > 5 && a.x < 1;
// expected-warning@-1{{overlapping comparisons always evaluate to false}}
return a.y == 1 || a.y != 1;
// expected-warning@-1{{overlapping comparisons always evaluate to true}}
}
@@ -8,12 +8,18 @@
#define ASSERT_TYPE(...) static_assert(__is_same(__VA_ARGS__))
#define ASSERT_EXPR_TYPE(Expr, Expect) static_assert(__is_same(decltype(Expr), Expect));

struct S {
static int x[5];
};

void self_compare() {
int a;
int *b = nullptr;
S s;

(void)(a <=> a); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
(void)(b <=> b); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
(void)(s.x[a] <=> S::x[a]); // expected-warning {{self-comparison always evaluates to 'std::strong_ordering::equal'}}
}

void test0(long a, unsigned long b) {

0 comments on commit 4c05de8

Please sign in to comment.
You can’t perform that action at this time.