Author: Dávid Bolvanský Date: 2021-10-03T11:06:40+02:00 New Revision: f62d18ff140f67a8776a7a3c62a75645d8d540b5
URL: https://github.com/llvm/llvm-project/commit/f62d18ff140f67a8776a7a3c62a75645d8d540b5 DIFF: https://github.com/llvm/llvm-project/commit/f62d18ff140f67a8776a7a3c62a75645d8d540b5.diff LOG: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with 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 Added: clang/test/Sema/warn-bitwise-and-bool.c clang/test/Sema/warn-bitwise-or-bool.c Modified: clang/include/clang/Basic/DiagnosticGroups.td clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaChecking.cpp clang/test/Misc/warning-wall.c Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 761b323d06166..d9db3482dbda7 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -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">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 1d4ea92c65205..c71a00b184328 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -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< @@ -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?">, diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp index 8b53e3504e13a..10a3f30704172 100644 --- a/clang/lib/Sema/SemaChecking.cpp +++ b/clang/lib/Sema/SemaChecking.cpp @@ -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)) { diff --git a/clang/test/Misc/warning-wall.c b/clang/test/Misc/warning-wall.c index a3686fb96a4ce..a4a79bec934af 100644 --- a/clang/test/Misc/warning-wall.c +++ b/clang/test/Misc/warning-wall.c @@ -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 diff --git a/clang/test/Sema/warn-bitwise-and-bool.c b/clang/test/Sema/warn-bitwise-and-bool.c new file mode 100644 index 0000000000000..6bec1be1abdef --- /dev/null +++ b/clang/test/Sema/warn-bitwise-and-bool.c @@ -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)); +} diff --git a/clang/test/Sema/warn-bitwise-or-bool.c b/clang/test/Sema/warn-bitwise-or-bool.c new file mode 100644 index 0000000000000..ae86790901aac --- /dev/null +++ b/clang/test/Sema/warn-bitwise-or-bool.c @@ -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)); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
