Missing tautological compare warnings due to unary operators

[Codesbyusman]

Summary

The patch mainly focuses on the no warnings for -Wtautological-compare. I t work fine for the positive numbers but doesn't for the negative numbers as mention in the post that this is because the warning explicitly checks for an IntegerLiteral AST node, but -1 is represented by a UnaryOperator with an IntegerLiteral sub-Expr.
So, for the solution lib/Analysis/CFG.cpp was updated by using the different hellper functions to identify that it is a integer lliteral and then will take value from the expression. The functions checkIncorrectEqualityOperator and also functions for the relational operators will be updated.

For the below code we have warnings:

if (0 == (5 | x)) {}

but not for

if (0 == (-5 | x)) {}

Thus updating to tackle this issue.

GitHub Issue

[Codesbyusman]

updating to more efficient

[Codesbyusman]

also updating for the Xor bitwise operator

[aaron.ballman]

Thanks for working on this! I have some suggestions, but you'll also need to add a release note at some point.

clang/lib/Analysis/CFG.cpp

975–982

Coding style fix.

986

Be sure to add test coverage for this change.

986–988

989–996

(Be sure to re-run clang format over the patch, in case I've messed up 80-col limits with my suggestions.)

995

It looks like you've added a spurious tab here.

997–998

Take this suggestion or leave it, as you want. I don't think the comment added much value because it's describing what the code does (which is reasonably obvious from reading the code given that the code is pretty simple) rather than describing why the code is necessary (which doesn't seem to be required in this case).

1005

This looks like an existing bug and suggests we're missing test coverage -- we create a TryResult object but do nothing with it; I suspect we wanted to return this result.

Can you try to add test coverage that hits this code path to verify the current behavior is wrong, then change it to return the result to make sure the behavior is corrected?

1007–1010

clang/test/SemaCXX/warn-unreachable.cpp

398–399

Why aren't we catching this one?

411–412

Or this one?

[Codesbyusman]

clang/lib/Analysis/CFG.cpp

975–982

ooh .. nice will update it

986

Yes working on that

1005

ok will try to make a test that will cover it

[Codesbyusman]

updating the code suggestions and test cases.

[aaron.ballman]

Thank you for this, I think this is good incremental progress and is almost ready to go. Just a few small nits, but also, can you also add a release note for the fix (be sure to mention which issue is being closed too).

Note, the precommit CI failures are unrelated to the changes in this patch.

clang/test/Sema/warn-bitwise-compare.c

42

85

clang/test/SemaCXX/warn-unreachable.cpp

399–410

We should move these up above the TODO comment on line 399, since these are now getting the expected warnings.

[Codesbyusman]

In D130510#3682902, @aaron.ballman wrote:

Thank you for this, I think this is good incremental progress and is almost ready to go. Just a few small nits, but also, can you also add a release note for the fix (be sure to mention which issue is being closed too).

Note, the precommit CI failures are unrelated to the changes in this patch.

where should I write this release note. Like in comment or somewhere else?

[aaron.ballman]

In D130510#3683078, @Codesbyusman wrote:

In D130510#3682902, @aaron.ballman wrote:

Thank you for this, I think this is good incremental progress and is almost ready to go. Just a few small nits, but also, can you also add a release note for the fix (be sure to mention which issue is being closed too).

Note, the precommit CI failures are unrelated to the changes in this patch.

where should I write this release note. Like in comment or somewhere else?

All of Clang's release notes live in: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst and you'd want to put your note under Improvements to Clang's diagnostics (since you're making the diagnostic behavior better) or Bug Fixes (since there's a GitHub issue associated with it). I'd probably list it under Bug Fixes.

[Codesbyusman]

updated with the small fixes and addition of release note

[aaron.ballman]

LGTM! I'll make some editorial corrections for grammar to the release note when I land.

[Codesbyusman]

In D130510#3684740, @aaron.ballman wrote:

LGTM! I'll make some editorial corrections for grammar to the release note when I land.

Thank you.

[rtrieu]

This warning is now flagging some code which I believe is a false positive. In particular, when template arguments are involved, their values can be calculated during template instantiation, but they should be treated as variables instead of constants. For example:

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;
}

void run(int x) {
    foo<4, int>(8);
}

In this instantiation, x is and'ed with the value 4 in different ways. sizeof(T) is type-dependent, I is value-dependent, and 4 is an integer literal. With your code, each of them would produce a warning. However, the first two values of 4 will change in different template instantiations, so the warning is not useful when it is okay for some instantiations to have constant values. Because of this, warnings should treat dependent expressions as non-constant even when they can be evaluated, so only b3 should get a warning. This is causing the warning to be emitted on code heavy in template meta-programming, such as array libraries. Please revert or fix.

I believe that evaluating the expression would make this warning too broad and would need more testing that what was included here. Only handling UnaryOperator with IntegerLiteral sub-expression makes more sense for the warning, and adding in any new cases if we find them.

[aaron.ballman]

In D130510#3692654, @rtrieu wrote:

This warning is now flagging some code which I believe is a false positive. In particular, when template arguments are involved, their values can be calculated during template instantiation, but they should be treated as variables instead of constants. For example:

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;
}

void run(int x) {
    foo<4, int>(8);
}

In this instantiation, x is and'ed with the value 4 in different ways. sizeof(T) is type-dependent, I is value-dependent, and 4 is an integer literal. With your code, each of them would produce a warning. However, the first two values of 4 will change in different template instantiations, so the warning is not useful when it is okay for some instantiations to have constant values.

Yeah, this is definitely a false positive we need to avoid, thank you for bringing it up!

Because of this, warnings should treat dependent expressions as non-constant even when they can be evaluated, so only b3 should get a warning. This is causing the warning to be emitted on code heavy in template meta-programming, such as array libraries. Please revert or fix.

Yeah, I agree. Unfortunately, the CFG makes this exceptionally difficult because it walks over the instantiated code, so we've lost that the original expression was dependent by the time we get to checking the binary expressions. The original code worked by virtue of overfitting to *just* integer literals.

I believe that evaluating the expression would make this warning too broad and would need more testing that what was included here. Only handling UnaryOperator with IntegerLiteral sub-expression makes more sense for the warning, and adding in any new cases if we find them.

I agree that the warning is too broad right now and that's unintentional. However, manually handling every single case in the CFG as something special is fragile and what got us this bug in the first place. We have a constant expression evaluator (two, actually) and we shouldn't have to reimplement it a third time in the CFG. However, asking a GSoC mentee to address that is well beyond the scope of what they should be working on. So for now I'm going to revert this change, reopen the issue, and we'll discuss the next steps off-list with Usman.

[aaron.ballman]

I've reverted in c783ca0de1e1e00f364cf4745b8444a020ddd29b. Marking as requesting changes to make it clear more work needs to be done.

[rtrieu]

In D130510#3693494, @aaron.ballman wrote:

In D130510#3692654, @rtrieu wrote:

Because of this, warnings should treat dependent expressions as non-constant even when they can be evaluated, so only b3 should get a warning. This is causing the warning to be emitted on code heavy in template meta-programming, such as array libraries. Please revert or fix.

Yeah, I agree. Unfortunately, the CFG makes this exceptionally difficult because it walks over the instantiated code, so we've lost that the original expression was dependent by the time we get to checking the binary expressions. The original code worked by virtue of overfitting to *just* integer literals.

Not being able to detect when expressions are dependent inside template instantiations has been a pain for warnings since forever.

I believe that evaluating the expression would make this warning too broad and would need more testing that what was included here. Only handling UnaryOperator with IntegerLiteral sub-expression makes more sense for the warning, and adding in any new cases if we find them.

I agree that the warning is too broad right now and that's unintentional. However, manually handling every single case in the CFG as something special is fragile and what got us this bug in the first place. We have a constant expression evaluator (two, actually) and we shouldn't have to reimplement it a third time in the CFG. However, asking a GSoC mentee to address that is well beyond the scope of what they should be working on. So for now I'm going to revert this change, reopen the issue, and we'll discuss the next steps off-list with Usman.

I agree, the CFG should be as broadly applicable as possible, so using an evaluator there is fine. Manually handling every single case may be needed to keep the warning under control. It's possible to put that handling to the Sema side of things, right before the warning is emitted. There's already a filter for macros, so maybe adding the filtering logic there would be a good fix.

[Codesbyusman]

Adopted another approach working for the error caught. Kindly review this.

[rtrieu]

Can you add my earlier test case or something like it to SemaCXX/warn-bitwise-compare.cpp ?

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;  // only warn here
}

void run(int x) {
    foo<4, int>(8);
}

clang/lib/Analysis/CFG.cpp

58

For debugging?

Have you tried llvm::errs() << "message"; ? A few streams are provided by LLVM support which most places have already, so no extra header is needed to make it work.

1020

Is this any better than just having the callers use EvaluateAsInt themselves?

[Codesbyusman]

In D130510#3698415, @rtrieu wrote:

Can you add my earlier test case or something like it to SemaCXX/warn-bitwise-compare.cpp ?

template <int I, class T>
void foo(int x) {
    bool b1 = (x & sizeof(T)) == 8;
    bool b2 = (x & I) == 8;
    bool b3 = (x & 4) == 8;  // only warn here
}

void run(int x) {
    foo<4, int>(8);
}

Yes sure, will update you

clang/lib/Analysis/CFG.cpp

58

For debugging?

Yes. I just forget to remove them. Will updaye them.

Have you tried llvm::errs() << "message"; ? A few streams are provided by LLVM support which most places have already, so no extra header is needed to make it work.

okay, Great. Will use in future. Thank you.

[rtrieu]

void foo(long x) {
  if ((x & 1) == 1L) return;  // bad always false warning here
  static_assert(sizeof(int) < sizeof(long), "long is bigger than int");
  static_assert((long(15) & 1) == 1L, "proof that condition can be true");
}

I found this false positive case when testing your new patch. The condition is fine, but it gives an always false warning. When fixed, this would be another good test case to include.

[Codesbyusman]

updating for the missing tautological compare warnings.

[Codesbyusman]

updates

[Codesbyusman]

updated also the test cases.

[Codesbyusman]

updated.

[aaron.ballman]

This is looking much closer to what I think we had in mind, so I mostly have some cleanup suggestions.

clang/lib/Analysis/CFG.cpp

983

You can drop the IgnoreParens() here -- BoolExpr is either LHSExpr or RHSExpr, and both of those were initialized from a call to IgnoreParens().

1017–1019

1021–1023

Removing an unnecessary variable.

1024–1025

Coding style nit -- if the type is spelled out explicitly in the initialization, we tend to prefer using auto for the type so we don't have to repeat the type information twice.

Also removes a declaration that's not needed.

1031

This ensures everything past here doesn't have to care about parens.

1032–1035

1036

1037–1038

1045–1049

1051

If there's a unary expression operating on an integer literal... it has to be one of the listed operations above, otherwise I don't think you can get here. So might as well assert and bail out rather than return a value that we didn't properly perform the unary operation on.

1054–1062

[Codesbyusman]

updated.

[aaron.ballman]

Nice! Just a few more small nits to fix that I can see.

clang/lib/Analysis/CFG.cpp

1020–1025

Oops, I realized we could simplify this further and keep UnOp scoped more tightly to where it's used.

1030–1036

Another simplification to scope things more tightly.

1049–1051

[Codesbyusman]

updated.

[aaron.ballman]

This looks correct to me! Any further concerns @erichkeane or @rtrieu?

[rtrieu]

This patch has been moving back and forth between IsIntegerLiteralConstantExpr and getIntegerLiteralSubexpressionValue. The first function is preexisting and the second one is a new function. The final patch seems to settle on using just getIntegerLiteralSubexpressionValue. Can you explain why the existing function does not meet your needs? It wasn't clear from the update messages why you went that way.

Besides that, there is added support for multiple unary operators, but only minus is tested. Each one should have at least a positive and a negative test to show it is supported.

[aaron.ballman]

In D130510#3727096, @rtrieu wrote:

This patch has been moving back and forth between IsIntegerLiteralConstantExpr and getIntegerLiteralSubexpressionValue. The first function is preexisting and the second one is a new function. The final patch seems to settle on using just getIntegerLiteralSubexpressionValue. Can you explain why the existing function does not meet your needs? It wasn't clear from the update messages why you went that way.

The existing function returns whether the expression is an ICE (sort of), the replacement function calculates the actual value and returns an optional APInt so you can perform operations on it directly. That said, a future refactoring could probably remove IsIntegerLiteralConstantExpr() and replace it with getIntegerLiteralSubexpressionValue(), but the only caller of IsIntegerLiteralConstantExpr() doesn't actually need the value at that point.

Besides that, there is added support for multiple unary operators, but only minus is tested. Each one should have at least a positive and a negative test to show it is supported.

Good catch! I agree those tests should be added.

[Codesbyusman]

working on the test. Will update the patch soon.

[Codesbyusman]

updated test cases.

[rtrieu]

In D130510#3728719, @aaron.ballman wrote:

In D130510#3727096, @rtrieu wrote:

This patch has been moving back and forth between IsIntegerLiteralConstantExpr and getIntegerLiteralSubexpressionValue. The first function is preexisting and the second one is a new function. The final patch seems to settle on using just getIntegerLiteralSubexpressionValue. Can you explain why the existing function does not meet your needs? It wasn't clear from the update messages why you went that way.

The existing function returns whether the expression is an ICE (sort of), the replacement function calculates the actual value and returns an optional APInt so you can perform operations on it directly. That said, a future refactoring could probably remove IsIntegerLiteralConstantExpr() and replace it with getIntegerLiteralSubexpressionValue(), but the only caller of IsIntegerLiteralConstantExpr() doesn't actually need the value at that point.

In that case, I suggest adding a comment to pointing to the other function. I expected that some day, someone will notice that the calculation for literals is slightly different between the two warnings and we should be helpful to them. I have no other concerns.

[aaron.ballman]

In D130510#3729864, @rtrieu wrote:

In D130510#3728719, @aaron.ballman wrote:

In D130510#3727096, @rtrieu wrote:

This patch has been moving back and forth between IsIntegerLiteralConstantExpr and getIntegerLiteralSubexpressionValue. The first function is preexisting and the second one is a new function. The final patch seems to settle on using just getIntegerLiteralSubexpressionValue. Can you explain why the existing function does not meet your needs? It wasn't clear from the update messages why you went that way.

The existing function returns whether the expression is an ICE (sort of), the replacement function calculates the actual value and returns an optional APInt so you can perform operations on it directly. That said, a future refactoring could probably remove IsIntegerLiteralConstantExpr() and replace it with getIntegerLiteralSubexpressionValue(), but the only caller of IsIntegerLiteralConstantExpr() doesn't actually need the value at that point.

In that case, I suggest adding a comment to pointing to the other function. I expected that some day, someone will notice that the calculation for literals is slightly different between the two warnings and we should be helpful to them. I have no other concerns.

That's a good suggestion, I've added two suggested edits for the comment. Thanks for the extra set of eyes on this review @rtrieu!

clang/lib/Analysis/CFG.cpp

74–79

1019–1020

[Codesbyusman]

updated.

[ebevhan]

Hi! A bit of late feedback on this patch. We found a failure in our downstream testing likely originating from here.

The failing case is:

void f(int a)
{
    (0 != (a | !0LL));
}

built with clang -cc1 -emit-llvm-bc -O2 -v -o foo.bc -x c foo.c

clang/lib/Analysis/CFG.cpp

1044

This isn't returning an APInt of the right width. It will construct an APInt with a width of the input value, but that isn't the same as the required width of what a logical not produces; that should have a width of 'int'.

[aaron.ballman]

In D130510#3817148, @ebevhan wrote:

Hi! A bit of late feedback on this patch. We found a failure in our downstream testing likely originating from here.

Thank you for the feedback! I've addressed the issue in 96a79cb308d1b8c00a83b180d9fecc5d54bacb9c.

clang/lib/Analysis/CFG.cpp

1044

Good catch, that is correct -- logical not always returns an int.

[uabelho]

In D130510#3821641, @aaron.ballman wrote:

In D130510#3817148, @ebevhan wrote:

Hi! A bit of late feedback on this patch. We found a failure in our downstream testing likely originating from here.

Thank you for the feedback! I've addressed the issue in 96a79cb308d1b8c00a83b180d9fecc5d54bacb9c.

Thanks!

clang/docs/ReleaseNotes.rst

- Fixed a crash-on-valid with consteval evaluation of a list-initialized
   `Issue 55871 <https://github.com/llvm/llvm-project/issues/55871>`_.
 - Fix `#57008 <https://github.com/llvm/llvm-project/issues/57008>`_ - Builtin
   C++ language extension type traits instantiated by a template with unexpected
   number of arguments cause an assertion fault.
 - Fix multi-level pack expansion of undeclared function parameters.
   This fixes `Issue 56094 <https://github.com/llvm/llvm-project/issues/56094>`_.
 - Fix `#57151 <https://github.com/llvm/llvm-project/issues/57151>`_.
   ``-Wcomma`` is emitted for void returning functions.
+- ``-Wtautological-compare`` missed warnings for tautological comparisons
+  involving a negative integer literal. This fixes
+  `Issue 42918 <https://github.com/llvm/llvm-project/issues/42918>`_.
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now correctly diagnose as ill-formed a constant expression where an
   enum without a fixed underlying type is set to a value outside the range of
   the enumeration's values. Due to the extended period of time this bug was
   present in major C++ implementations (including Clang), this error has the
   ability to be downgraded into a warning (via: -Wno-error=enum-constexpr-conversion)

clang/lib/Analysis/CFG.cpp

 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/DOTGraphTraits.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/GraphWriter.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <memory>
 #include <string>
 #include <tuple>
 #include <utility>
 #include <vector>
 
 using namespace clang;
 
 static SourceLocation GetEndLoc(Decl *D) {
   if (VarDecl *VD = dyn_cast<VarDecl>(D))
     if (Expr *Ex = VD->getInit())
       return Ex->getSourceRange().getEnd();
   return D->getLocation();
 }
 
 /// Returns true on constant values based around a single IntegerLiteral.
 /// Allow for use of parentheses, integer casts, and negative signs.
+/// FIXME: it would be good to unify this function with
+/// getIntegerLiteralSubexpressionValue at some point given the similarity
+/// between the functions.
+
 static bool IsIntegerLiteralConstantExpr(const Expr *E) {
   // Allow parentheses
   E = E->IgnoreParens();
 
   // Allow conversions to different integer kind.
   if (const auto *CE = dyn_cast<CastExpr>(E)) {
     if (CE->getCastKind() != CK_IntegralCast)
       return false;
     E = CE->getSubExpr();
private:
   /// Find an incorrect equality comparison. Either with an expression
   /// evaluating to a boolean and a constant other than 0 and 1.
   /// e.g. if (!x == 10) or a bitwise and/or operation that always evaluates to
   /// true/false e.q. (x & 8) == 4.
   TryResult checkIncorrectEqualityOperator(const BinaryOperator *B) {
     const Expr *LHSExpr = B->getLHS()->IgnoreParens();
     const Expr *RHSExpr = B->getRHS()->IgnoreParens();
 
-    const IntegerLiteral *IntLiteral = dyn_cast<IntegerLiteral>(LHSExpr);
+    Optional<llvm::APInt> IntLiteral1 =
+        getIntegerLiteralSubexpressionValue(LHSExpr);
     const Expr *BoolExpr = RHSExpr;
 
-    if (!IntLiteral) {
-      IntLiteral = dyn_cast<IntegerLiteral>(RHSExpr);
+    if (!IntLiteral1) {
+      IntLiteral1 = getIntegerLiteralSubexpressionValue(RHSExpr);
       BoolExpr = LHSExpr;
     }
 
-    if (!IntLiteral)
+    if (!IntLiteral1)
       return TryResult();
 
     const BinaryOperator *BitOp = dyn_cast<BinaryOperator>(BoolExpr);
     if (BitOp && (BitOp->getOpcode() == BO_And ||
                   BitOp->getOpcode() == BO_Or)) {
       const Expr *LHSExpr2 = BitOp->getLHS()->IgnoreParens();
       const Expr *RHSExpr2 = BitOp->getRHS()->IgnoreParens();
 
-      const IntegerLiteral *IntLiteral2 = dyn_cast<IntegerLiteral>(LHSExpr2);
+      Optional<llvm::APInt> IntLiteral2 =
+          getIntegerLiteralSubexpressionValue(LHSExpr2);
 
       if (!IntLiteral2)
-        IntLiteral2 = dyn_cast<IntegerLiteral>(RHSExpr2);
+        IntLiteral2 = getIntegerLiteralSubexpressionValue(RHSExpr2);
 
       if (!IntLiteral2)
         return TryResult();
 
-      llvm::APInt L1 = IntLiteral->getValue();
-      llvm::APInt L2 = IntLiteral2->getValue();
-      if ((BitOp->getOpcode() == BO_And && (L2 & L1) != L1) ||
-          (BitOp->getOpcode() == BO_Or  && (L2 | L1) != L1)) {
+      if ((BitOp->getOpcode() == BO_And &&
+           (*IntLiteral2 & *IntLiteral1) != *IntLiteral1) ||
+          (BitOp->getOpcode() == BO_Or &&
+           (*IntLiteral2 | *IntLiteral1) != *IntLiteral1)) {
         if (BuildOpts.Observer)
           BuildOpts.Observer->compareBitwiseEquality(B,
                                                      B->getOpcode() != BO_EQ);
         TryResult(B->getOpcode() != BO_EQ);
       }
     } else if (BoolExpr->isKnownToHaveBooleanValue()) {
-      llvm::APInt IntValue = IntLiteral->getValue();
-      if ((IntValue == 1) || (IntValue == 0)) {
+      if ((*IntLiteral1 == 1) || (*IntLiteral1 == 0)) {
         return TryResult();
       }
       return TryResult(B->getOpcode() != BO_EQ);
     }
 
     return TryResult();
   }
 
+  // Helper function to get an APInt from an expression. Supports expressions
+  // which are an IntegerLiteral or a UnaryOperator and returns the value with
+  // all operations performed on it.
+  // FIXME: it would be good to unify this function with
+  // IsIntegerLiteralConstantExpr at some point given the similarity between the
+  // functions.
+  Optional<llvm::APInt> getIntegerLiteralSubexpressionValue(const Expr *E) {
+
+    // If unary.
+    if (const auto *UnOp = dyn_cast<UnaryOperator>(E->IgnoreParens())) {
+      // Get the sub expression of the unary expression and get the Integer
+      // Literal.
+      const Expr *SubExpr = UnOp->getSubExpr()->IgnoreParens();
+
+      if (const auto *IntLiteral = dyn_cast<IntegerLiteral>(SubExpr)) {
+
+        llvm::APInt Value = IntLiteral->getValue();
+
+        // Perform the operation manually.
+        switch (UnOp->getOpcode()) {
+        case UO_Plus:
+          return Value;
+        case UO_Minus:
+          return -Value;
+        case UO_Not:
+          return ~Value;
+        case UO_LNot:
+          return llvm::APInt(Value.getBitWidth(), !Value);
+        default:
+          assert(false && "Unexpected unary operator!");
+          return llvm::None;
+        }
+      }
+    } else if (const auto *IntLiteral =
+                   dyn_cast<IntegerLiteral>(E->IgnoreParens()))
+      return IntLiteral->getValue();
+
+    return llvm::None;
+  }
+
   TryResult analyzeLogicOperatorCondition(BinaryOperatorKind Relation,
                                           const llvm::APSInt &Value1,
                                           const llvm::APSInt &Value2) {
     assert(Value1.isSigned() == Value2.isSigned());
     switch (Relation) {
       default:
         return TryResult();
       case BO_EQ:
         return TryResult(Value1 == Value2);
       case BO_NE:
         return TryResult(Value1 != Value2);
       case BO_LT:
         return TryResult(Value1 <  Value2);
       case BO_LE:

clang/test/Sema/warn-bitwise-compare.c

 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 #define mydefine 2
+#define mydefine2 -2
 
 enum {
   ZERO,
   ONE,
 };
 
 void f(int x) {
   if ((8 & x) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x & 8) == 4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((-8 & x) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x & -8) == 4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((x & 8) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((2 & x) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((x & -8) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((-2 & x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+
   if ((x | 4) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
   if ((x | 3) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
   if ((5 | x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+
+  if ((x | -4) == 3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x | -3) != 4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((-5 | x) != 3) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+
   if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x & 0xFFEB) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((0xFFDD | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
   if (!!((8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if (!!((-8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+
   int y = ((8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+  y = ((-8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+  y = ((3 | x) != 5) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to true}}
+  y = ((-3 | x) != 5) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to true}}
+
+  if ((x & 0) != !0) {} // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((x & 0) == !0) {} // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x & 2) == !0) {} // expected-warning {{bitwise comparison always evaluates to false}}
+
+  if ((x | 4) == +3) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  if ((x | 3) != +4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+
+  if ((x & 8) != ~4) {}  // expected-warning {{bitwise comparison always evaluates to true}}
+  if ((x & 0) == ~4) {}  // expected-warning {{bitwise comparison always evaluates to false}}
 
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
   if ((x | 4) != 4) {}
 
+  if ((x | 4) == +4) {}
+  if ((x | 4) != +4) {}
+
+  if ((x & 1) == !12) {}
+  if ((x | 0) == !0) {}
+
+  if ((x | 1) == ~4) {}
+  if ((x | 1) != ~0) {}
+
+  if ((-2 & x) != 4) {}
+  if ((x & -8) == -8) {}
+  if ((x & -8) != -8) {}
+  if ((x | -4) == -4) {}
+  if ((x | -4) != -4) {}
+
   if ((x & 9) == 8) {}
   if ((x & 9) != 8) {}
   if ((x | 4) == 5) {}
   if ((x | 4) != 5) {}
 
+  if ((x & -9) == -10) {}
+  if ((x & -9) != -10) {}
+  if ((x | -4) == -3) {}
+  if ((x | -4) != -3) {}
+
+  if ((x^0) == 0) {}
+
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
+
+  if ((x & mydefine2) == 8) {}
+  if ((x | mydefine2) == 4) {}
+
+  if ((x & 1) == 1L) {}
 }
 
 void g(int x) {
   if (x | 5) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
   if (5 | x) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
   if (!((x | 5))) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
 
   if (x | -1) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
   if (x | ONE) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
 
   if (x | ZERO) {}
 }

clang/test/SemaCXX/warn-bitwise-compare.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 // RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused %s
 
 void test(int x) {
   bool b1 = (8 & x) == 3;
   // expected-warning@-1 {{bitwise comparison always evaluates to false}}
   bool b2 = x | 5;
   // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
   bool b3 = (x | 5);
   // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
   bool b4 = !!(x | 5);
   // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
 }
+
+template <int I, class T>  // silence
+void foo(int x) {
+    bool b1 = (x & sizeof(T)) == 8;
+    bool b2 = (x & I) == 8;
+    bool b3 = (x & 4) == 8; // expected-warning {{bitwise comparison always evaluates to false}}
+}
+
+void run(int x) {
+    foo<4, int>(8); // expected-note {{in instantiation of function template specialization 'foo<4, int>' requested here}}
+}

clang/test/SemaCXX/warn-unreachable.cpp

void tautological_compare(bool x, int y) {
 
   if (y < 10 || y > 5)  // expected-note {{silence}}
     calledFun();
   else
     calledFun();        // expected-warning {{will never be executed}}
 
   if (y == -1 && y != -1)  // expected-note {{silence}}
     calledFun();        // expected-warning {{will never be executed}}
 
-  // TODO: Extend warning to the following code:
-  if (x < -1)
-    calledFun();
-  if (x == -1)
-    calledFun();
+  if (x == -1)   // expected-note {{silence}}
+    calledFun(); // expected-warning {{will never be executed}}
 
-  if (x != -1)
+  if (x != -1)   // expected-note {{silence}}
     calledFun();
   else
+    calledFun(); // expected-warning {{will never be executed}}
+
+  // TODO: Extend warning to the following code:
+  if (x < -1)
     calledFun();
+
   if (-1 > x)
     calledFun();
   else
     calledFun();
 
 }