From 83b598c14f054293c7055bb834a53161be627fa1 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Tue, 27 Jul 2010 18:49:08 +0000 Subject: [PATCH] Finesse 'idempotent operations' analyzer issues to include the opcode of the binary operator for clearer error reporting. Also remove the 'Idempotent operation' prefix in messages; it's redundant since the bug type is the same. llvm-svn: 109527 --- .../Checker/IdempotentOperationChecker.cpp | 26 +++++--- clang/test/Analysis/constant-folding.c | 10 +-- clang/test/Analysis/dead-stores.c | 4 +- clang/test/Analysis/idempotent-operations.c | 62 +++++++++---------- clang/test/Analysis/misc-ps.m | 4 +- clang/test/Analysis/null-deref-ps.c | 2 +- clang/test/Analysis/rdar-6541136-region.c | 4 +- clang/test/Analysis/rdar-6541136.c | 2 +- 8 files changed, 60 insertions(+), 54 deletions(-) diff --git a/clang/lib/Checker/IdempotentOperationChecker.cpp b/clang/lib/Checker/IdempotentOperationChecker.cpp index 95d488c6af62..f8bf708e5fda 100644 --- a/clang/lib/Checker/IdempotentOperationChecker.cpp +++ b/clang/lib/Checker/IdempotentOperationChecker.cpp @@ -310,7 +310,7 @@ void IdempotentOperationChecker::PreVisitBinaryOperator( } void IdempotentOperationChecker::VisitEndAnalysis(ExplodedGraph &G, - BugReporter &B, + BugReporter &BR, bool hasWorkRemaining) { // If there is any work remaining we cannot be 100% sure about our warnings if (hasWorkRemaining) @@ -322,22 +322,29 @@ void IdempotentOperationChecker::VisitEndAnalysis(ExplodedGraph &G, hash.begin(); i != hash.end(); ++i) { if (i->second != Impossible) { // Select the error message. - const char *msg = 0; + const BinaryOperator *B = i->first; + llvm::SmallString<128> buf; + llvm::raw_svector_ostream os(buf); + switch (i->second) { case Equal: - msg = "idempotent operation; both operands are always equal in value"; + if (B->getOpcode() == BinaryOperator::Assign) + os << "Assigned value is always the same as the existing value"; + else + os << "Both operands to '" << B->getOpcodeStr() + << "' always have the same value"; break; case LHSis1: - msg = "idempotent operation; the left operand is always 1"; + os << "The left operand to '" << B->getOpcodeStr() << "' is always 1"; break; case RHSis1: - msg = "idempotent operation; the right operand is always 1"; + os << "The right operand to '" << B->getOpcodeStr() << "' is always 1"; break; case LHSis0: - msg = "idempotent operation; the left operand is always 0"; + os << "The left operand to '" << B->getOpcodeStr() << "' is always 0"; break; case RHSis0: - msg = "idempotent operation; the right operand is always 0"; + os << "The right operand to '" << B->getOpcodeStr() << "' is always 0"; break; case Possible: llvm_unreachable("Operation was never marked with an assumption"); @@ -348,9 +355,8 @@ void IdempotentOperationChecker::VisitEndAnalysis(ExplodedGraph &G, // Create the SourceRange Arrays SourceRange S[2] = { i->first->getLHS()->getSourceRange(), i->first->getRHS()->getSourceRange() }; - B.EmitBasicReport("Idempotent operation", "Dead code", - msg, i->first->getOperatorLoc(), - S, 2); + BR.EmitBasicReport("Idempotent operation", "Dead code", + os.str(), i->first->getOperatorLoc(), S, 2); } } } diff --git a/clang/test/Analysis/constant-folding.c b/clang/test/Analysis/constant-folding.c index 5a7e6be1dc17..b1ca3eeec989 100644 --- a/clang/test/Analysis/constant-folding.c +++ b/clang/test/Analysis/constant-folding.c @@ -18,10 +18,10 @@ void testComparisons (int a) { } void testSelfOperations (int a) { - if ((a|a) != a) WARN; // expected-warning{{idempotent operation}} - if ((a&a) != a) WARN; // expected-warning{{idempotent operation}} - if ((a^a) != 0) WARN; // expected-warning{{idempotent operation}} - if ((a-a) != 0) WARN; // expected-warning{{idempotent operation}} + if ((a|a) != a) WARN; // expected-warning{{Both operands to '|' always have the same value}} + if ((a&a) != a) WARN; // expected-warning{{Both operands to '&' always have the same value}} + if ((a^a) != 0) WARN; // expected-warning{{Both operands to '^' always have the same value}} + if ((a-a) != 0) WARN; // expected-warning{{Both operands to '-' always have the same value}} } void testIdempotent (int a) { @@ -68,5 +68,5 @@ void testLocations (char *a) { if (b!=a) WARN; if (b>a) WARN; if (b @@ -458,7 +458,7 @@ void rdar8014335() { // Note that the next value stored to 'i' is never executed // because the next statement to be executed is the 'break' // in the increment code of the first loop. - i = i * 3; // expected-warning{{Value stored to 'i' is never read}} expected-warning{{idempotent operation}} + i = i * 3; // expected-warning{{Value stored to 'i' is never read}} expected-warning{{The left operand to '*' is always 1}} } } diff --git a/clang/test/Analysis/idempotent-operations.c b/clang/test/Analysis/idempotent-operations.c index 94e972c5519c..74d31e72396c 100644 --- a/clang/test/Analysis/idempotent-operations.c +++ b/clang/test/Analysis/idempotent-operations.c @@ -9,47 +9,47 @@ void basic() { int x = 10, zero = 0, one = 1; // x op x - x = x; // expected-warning {{idempotent operation; both operands are always equal in value}} - test(x - x); // expected-warning {{idempotent operation; both operands are always equal in value}} - x -= x; // expected-warning {{idempotent operation; both operands are always equal in value}} + x = x; // expected-warning {{Assigned value is always the same as the existing value}} + test(x - x); // expected-warning {{Both operands to '-' always have the same value}} + x -= x; // expected-warning {{Both operands to '-=' always have the same value}} x = 10; // no-warning - test(x / x); // expected-warning {{idempotent operation; both operands are always equal in value}} - x /= x; // expected-warning {{idempotent operation; both operands are always equal in value}} + test(x / x); // expected-warning {{Both operands to '/' always have the same value}} + x /= x; // expected-warning {{Both operands to '/=' always have the same value}} x = 10; // no-warning - test(x & x); // expected-warning {{idempotent operation; both operands are always equal in value}} - x &= x; // expected-warning {{idempotent operation; both operands are always equal in value}} - test(x | x); // expected-warning {{idempotent operation; both operands are always equal in value}} - x |= x; // expected-warning {{idempotent operation; both operands are always equal in value}} + test(x & x); // expected-warning {{Both operands to '&' always have the same value}} + x &= x; // expected-warning {{Both operands to '&=' always have the same value}} + test(x | x); // expected-warning {{Both operands to '|' always have the same value}} + x |= x; // expected-warning {{Both operands to '|=' always have the same value}} // x op 1 - test(x * one); // expected-warning {{idempotent operation; the right operand is always 1}} - x *= one; // expected-warning {{idempotent operation; the right operand is always 1}} - test(x / one); // expected-warning {{idempotent operation; the right operand is always 1}} - x /= one; // expected-warning {{idempotent operation; the right operand is always 1}} + test(x * one); // expected-warning {{The right operand to '*' is always 1}} + x *= one; // expected-warning {{The right operand to '*=' is always 1}} + test(x / one); // expected-warning {{The right operand to '/' is always 1}} + x /= one; // expected-warning {{The right operand to '/=' is always 1}} // 1 op x - test(one * x); // expected-warning {{idempotent operation; the left operand is always 1}} + test(one * x); // expected-warning {{The left operand to '*' is always 1}} // x op 0 - test(x + zero); // expected-warning {{idempotent operation; the right operand is always 0}} - test(x - zero); // expected-warning {{idempotent operation; the right operand is always 0}} - test(x * zero); // expected-warning {{idempotent operation; the right operand is always 0}} - test(x & zero); // expected-warning {{idempotent operation; the right operand is always 0}} - test(x | zero); // expected-warning {{idempotent operation; the right operand is always 0}} - test(x ^ zero); // expected-warning {{idempotent operation; the right operand is always 0}} - test(x << zero); // expected-warning {{idempotent operation; the right operand is always 0}} - test(x >> zero); // expected-warning {{idempotent operation; the right operand is always 0}} + test(x + zero); // expected-warning {{The right operand to '+' is always 0}} + test(x - zero); // expected-warning {{The right operand to '-' is always 0}} + test(x * zero); // expected-warning {{The right operand to '*' is always 0}} + test(x & zero); // expected-warning {{The right operand to '&' is always 0}} + test(x | zero); // expected-warning {{The right operand to '|' is always 0}} + test(x ^ zero); // expected-warning {{The right operand to '^' is always 0}} + test(x << zero); // expected-warning {{The right operand to '<<' is always 0}} + test(x >> zero); // expected-warning {{The right operand to '>>' is always 0}} // 0 op x - test(zero + x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero - x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero / x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero * x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero & x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero | x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero ^ x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero << x); // expected-warning {{idempotent operation; the left operand is always 0}} - test(zero >> x); // expected-warning {{idempotent operation; the left operand is always 0}} + test(zero + x); // expected-warning {{The left operand to '+' is always 0}} + test(zero - x); // expected-warning {{The left operand to '-' is always 0}} + test(zero / x); // expected-warning {{The left operand to '/' is always 0}} + test(zero * x); // expected-warning {{The left operand to '*' is always 0}} + test(zero & x); // expected-warning {{The left operand to '&' is always 0}} + test(zero | x); // expected-warning {{The left operand to '|' is always 0}} + test(zero ^ x); // expected-warning {{The left operand to '^' is always 0}} + test(zero << x); // expected-warning {{The left operand to '<<' is always 0}} + test(zero >> x); // expected-warning {{The left operand to '>>' is always 0}} } void floats(float x) { diff --git a/clang/test/Analysis/misc-ps.m b/clang/test/Analysis/misc-ps.m index c2099b04a95c..be7f450de6d8 100644 --- a/clang/test/Analysis/misc-ps.m +++ b/clang/test/Analysis/misc-ps.m @@ -86,11 +86,11 @@ unsigned r6268365Aux(); void r6268365() { unsigned x = 0; - x &= r6268365Aux(); // expected-warning{{idempotent operation}} + x &= r6268365Aux(); // expected-warning{{The left operand to '&=' is always 0}} unsigned j = 0; if (x == 0) ++j; - if (x == 0) x = x / j; // expected-warning{{idempotent operation}} expected-warning{{idempotent operation}} + if (x == 0) x = x / j; // expected-warning{{Assigned value is always the same as the existing value}} expected-warning{{The right operand to '/' is always 1}} } void divzeroassume(unsigned x, unsigned j) { diff --git a/clang/test/Analysis/null-deref-ps.c b/clang/test/Analysis/null-deref-ps.c index 0b4153c8344d..9be73a8c0f20 100644 --- a/clang/test/Analysis/null-deref-ps.c +++ b/clang/test/Analysis/null-deref-ps.c @@ -280,7 +280,7 @@ void f12(HF12ITEM i, char *q) { // Test handling of translating between integer "pointers" and back. void f13() { int *x = 0; - if (((((int) x) << 2) + 1) >> 1) *x = 1; // expected-warning{{idempotent operation}} + if (((((int) x) << 2) + 1) >> 1) *x = 1; // expected-warning{{he left operand to '<<' is always 0}} } // PR 4759 - Attribute non-null checking by the analyzer was not correctly diff --git a/clang/test/Analysis/rdar-6541136-region.c b/clang/test/Analysis/rdar-6541136-region.c index c1734eec9baa..80c04ae8ca10 100644 --- a/clang/test/Analysis/rdar-6541136-region.c +++ b/clang/test/Analysis/rdar-6541136-region.c @@ -9,7 +9,7 @@ extern kernel_tea_cheese_t _wonky_gesticulate_cheese; void test1( void ) { kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese; struct load_wine *cmd = (void*) &wonky[1]; - cmd = cmd; // expected-warning{{idempotent operation}} + cmd = cmd; // expected-warning{{Assigned value is always the same as the existing value}} char *p = (void*) &wonky[1]; kernel_tea_cheese_t *q = &wonky[1]; // This test case tests both the RegionStore logic (doesn't crash) and @@ -21,7 +21,7 @@ void test1( void ) { void test1_b( void ) { kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese; struct load_wine *cmd = (void*) &wonky[1]; - cmd = cmd; // expected-warning{{idempotent operation}} + cmd = cmd; // expected-warning{{Assigned value is always the same as the existing value}} char *p = (void*) &wonky[1]; *p = 1; // expected-warning{{Access out-of-bound array element (buffer overflow)}} } diff --git a/clang/test/Analysis/rdar-6541136.c b/clang/test/Analysis/rdar-6541136.c index c729cb518fe1..911fb4aa2f30 100644 --- a/clang/test/Analysis/rdar-6541136.c +++ b/clang/test/Analysis/rdar-6541136.c @@ -12,7 +12,7 @@ void foo( void ) { kernel_tea_cheese_t *wonky = &_wonky_gesticulate_cheese; struct load_wine *cmd = (void*) &wonky[1]; - cmd = cmd; // expected-warning{{idempotent operation}} + cmd = cmd; // expected-warning{{Assigned value is always the same as the existing value}} char *p = (void*) &wonky[1]; *p = 1; kernel_tea_cheese_t *q = &wonky[1];