Skip to content

Commit

Permalink
[Clang] Extend -Wbool-operation to warn about bitwise and of bools wi…
Browse files Browse the repository at this point in the history
…th side effects

Motivation: https://arstechnica.com/gadgets/2021/07/google-pushed-a-one-character-typo-to-production-bricking-chrome-os-devices/

Warn for pattern boolA & boolB or boolA | boolB where boolA and boolB has possible side effects.

Casting one operand to int is enough to silence this warning: for example (int)boolA & boolB or boolA| (int)boolB

Fixes https://bugs.llvm.org/show_bug.cgi?id=51216

Differential Revision: https://reviews.llvm.org/D108003
  • Loading branch information
davidbolvansky committed Oct 3, 2021
1 parent cf284f6 commit f62d18f
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 1 deletion.
3 changes: 2 additions & 1 deletion clang/include/clang/Basic/DiagnosticGroups.td
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ def StringConversion : DiagGroup<"string-conversion">;
def SignConversion : DiagGroup<"sign-conversion">;
def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;
def UndefinedBoolConversion : DiagGroup<"undefined-bool-conversion">;
def BoolOperation : DiagGroup<"bool-operation">;
def BitwiseInsteadOfLogical : DiagGroup<"bitwise-instead-of-logical">;
def BoolOperation : DiagGroup<"bool-operation", [BitwiseInsteadOfLogical]>;
def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion,
UndefinedBoolConversion]>;
def IntConversion : DiagGroup<"int-conversion">;
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ def warn_infinite_recursive_function : Warning<
def warn_comma_operator : Warning<"possible misuse of comma operator here">,
InGroup<DiagGroup<"comma">>, DefaultIgnore;
def note_cast_to_void : Note<"cast expression to void to silence warning">;
def note_cast_operand_to_int : Note<"cast one or both operands to int to silence this warning">;

// Constant expressions
def err_expr_not_ice : Error<
Expand Down Expand Up @@ -7423,6 +7424,9 @@ def note_member_declared_here : Note<
"member %0 declared here">;
def note_member_first_declared_here : Note<
"member %0 first declared here">;
def warn_bitwise_instead_of_logical : Warning<
"use of bitwise '%0' with boolean operands">,
InGroup<BitwiseInsteadOfLogical>, DefaultIgnore;
def warn_bitwise_negation_bool : Warning<
"bitwise negation of a boolean expression%select{;| always evaluates to 'true';}0 "
"did you mean logical negation?">,
Expand Down
14 changes: 14 additions & 0 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13249,6 +13249,20 @@ static void AnalyzeImplicitConversions(
<< OrigE->getSourceRange() << T->isBooleanType()
<< FixItHint::CreateReplacement(UO->getBeginLoc(), "!");

if (const auto *BO = dyn_cast<BinaryOperator>(SourceExpr))
if ((BO->getOpcode() == BO_And || BO->getOpcode() == BO_Or) &&
BO->getLHS()->isKnownToHaveBooleanValue() &&
BO->getRHS()->isKnownToHaveBooleanValue() &&
BO->getLHS()->HasSideEffects(S.Context) &&
BO->getRHS()->HasSideEffects(S.Context)) {
S.Diag(BO->getBeginLoc(), diag::warn_bitwise_instead_of_logical)
<< (BO->getOpcode() == BO_And ? "&" : "|") << OrigE->getSourceRange()
<< FixItHint::CreateReplacement(
BO->getOperatorLoc(),
(BO->getOpcode() == BO_And ? "&&" : "||"));
S.Diag(BO->getBeginLoc(), diag::note_cast_operand_to_int);
}

// For conditional operators, we analyze the arguments as if they
// were being fed directly into the output.
if (auto *CO = dyn_cast<AbstractConditionalOperator>(SourceExpr)) {
Expand Down
1 change: 1 addition & 0 deletions clang/test/Misc/warning-wall.c
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ RUN: FileCheck --input-file=%t %s
CHECK:-Wall
CHECK-NEXT: -Wmost
CHECK-NEXT: -Wbool-operation
CHECK-NEXT: -Wbitwise-instead-of-logical
CHECK-NEXT: -Wchar-subscripts
CHECK-NEXT: -Wcomment
CHECK-NEXT: -Wdelete-non-virtual-dtor
Expand Down
63 changes: 63 additions & 0 deletions clang/test/Sema/warn-bitwise-and-bool.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s

#ifdef __cplusplus
typedef bool boolean;
#else
typedef _Bool boolean;
#endif

boolean foo(void);
boolean bar(void);
boolean baz(void) __attribute__((const));
void sink(boolean);

#define FOO foo()

void test(boolean a, boolean b, int *p, volatile int *q, int i) {
b = a & b;
b = foo() & a;
b = (p != 0) & (*p == 42); // FIXME: also warn for a non-volatile pointer dereference
b = foo() & (*q == 42); // expected-warning {{use of bitwise '&' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
b = foo() & (int)(*q == 42); // OK, no warning expected
b = a & foo();
b = (int)a & foo(); // OK, no warning expected
b = foo() & bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"&&"
b = foo() & (int)bar(); // OK, no warning expected
b = foo() & !bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"&&"
b = a & baz();
b = bar() & FOO; // expected-warning {{use of bitwise '&' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"&&"
b = foo() & (int)FOO; // OK, no warning expected
b = b & foo();
b = bar() & (i > 4);
b = (i == 7) & foo();
#ifdef __cplusplus
b = foo() bitand bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
#endif

if (foo() & bar()) // expected-warning {{use of bitwise '&' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
;

sink(a & b);
sink(a & foo());
sink(foo() & bar()); // expected-warning {{use of bitwise '&' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}

int n = i + 10;
b = (n & (n - 1));
}
63 changes: 63 additions & 0 deletions clang/test/Sema/warn-bitwise-or-bool.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s

#ifdef __cplusplus
typedef bool boolean;
#else
typedef _Bool boolean;
#endif

boolean foo(void);
boolean bar(void);
boolean baz(void) __attribute__((const));
void sink(boolean);

#define FOO foo()

void test(boolean a, boolean b, int *p, volatile int *q, int i) {
b = a | b;
b = foo() | a;
b = (p != 0) | (*p == 42); // FIXME: also warn for a non-volatile pointer dereference
b = foo() | (*q == 42); // expected-warning {{use of bitwise '|' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
b = foo() | (int)(*q == 42); // OK, no warning expected
b = a | foo();
b = (int)a | foo(); // OK, no warning expected
b = foo() | bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
b = foo() | !bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
b = foo() | (int)bar(); // OK, no warning expected
b = a | baz();
b = bar() | FOO; // expected-warning {{use of bitwise '|' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
// CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
b = foo() | (int)FOO; // OK, no warning expected
b = b | foo();
b = bar() | (i > 4);
b = (i == 7) | foo();
#ifdef __cplusplus
b = foo() bitor bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
#endif

if (foo() | bar()) // expected-warning {{use of bitwise '|' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}
;

sink(a | b);
sink(a | foo());
sink(foo() | bar()); // expected-warning {{use of bitwise '|' with boolean operands}}
// expected-note@-1 {{cast one or both operands to int to silence this warning}}

int n = i + 10;
b = (n | (n - 1));
}

0 comments on commit f62d18f

Please sign in to comment.