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
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Open questions:
1, warn for pattern boolA & boolB always? I think that we will have some false positives.
2, restrict this warning even more? RHS must be a call?
3, something similar for bitwise or?
@hans @nickdesaulniers maybe you want to try this new warning on Chrome/Linux kernel?
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 7434–7436 | I suggest that the name and wording of this diagnostic should match warn_logical_instead_of_bitwise, currently "use of logical '%0' with constant operand". So: def warn_bitwise_instead_of_logical : Warning< "use of bitwise '%0' with boolean operand">, This neatly sidesteps the problem of what to call the & operator: I was not thrilled with the phrase bitwise and of, but have no problem with use of bitwise '&'. | |
| clang/test/Sema/warn-bitwise-and-bool.c | ||
| 22–23 | So the warning triggers only when the RHS has side-effects? I'd like tests for foo() & a; // should not trigger, by that logic, because && wouldn't short-circuit anything? (p != nullptr) & (*p == 42); // should certainly trigger: deref is a side effect, and of course obviously this is a bug | |
| 32 | Why is this assigning to i instead of b? void sink(bool); sink(a & b); sink(a & foo()); sink(foo() & bar()); etc. | |
| 35 | I'd also like a negative test for @mclow.lists' example of int n = ...; b = (n & (n-1)); (where the result is being used as a boolean, but the bitwise op is correct and the logical op would be 100% wrong). | |
| 37 | I assume you meant bitand and expect a warning? | |
Comment Actions
I suggest you take all the techniques at http://graphics.stanford.edu/~seander/bithacks.html and make sure they don't cause a warning.
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 7434–7436 | I see the point but then I will not be able to provide -Wbool-operation-and flag to enable/disable this warning. For example I know that Google prefers a new flag for every new warning so they dont have to disable eg. -Wbool-operation, but just this new warning while there are working on fixes for new warnings. @hans what do you think? | |
Comment Actions
With several Linux kernel defconfig builds, I see the following instances of the warning:
drivers/net/wireless/intel/iwlwifi/mvm/scan.c:835:3: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] drivers/staging/rtl8192u/r8192U_core.c:4268:20: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:1043:7: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:1075:30: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:1294:7: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:1353:30: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:1932:7: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:1956:22: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:1977:23: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and] lib/zstd/compress.c:2024:29: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and]
Which correspond to:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/zstd/compress.c#n1294
I can do allmodconfig builds later today.
Comment Actions
Thanks!
Well, for those cases, && instead of & looks like a better choice. WDYT?
Or is it a very noisy? Restrict more so both sides must have side effects?
Maybe we could suggest to use cast - (int)boolRHS - as a way how to silence this warning.. Or any better suggestion?
Comment Actions
I ran this over Chrome and ran into a use case that looks legitimate. It seems like the pattern in LLVM where we want to run a bunch of transformations and get if any of them changed anything.
Comment Actions
FWIW, all three of @nathanchance's detected lines look like good true positives to me: even if they're not bugs, they all look like places the programmer meant to write && and only wrote & by accident. The third one might even be a bug bug, since it's doing essentially (bounds-check offset_1) & (pointer-math-on offset_1).
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/intel/iwlwifi/mvm/scan.c#n830
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtl8192u/r8192U_core.c#n4268
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/zstd/compress.c#n1294
Data point: I've run this patch over my employer's medium-sized mostly-modern codebase and found no (true or false) positives at all.
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 7434–7436 | I don't understand your comment. My guess is that you didn't notice that -Wbitwise-instead-of-logical would still be a different flag from -Wlogical-instead-of-bitwise. I'm not suggesting putting them under the same flag; just making the newly added flag a little more consistent and (heh) logical, based on what's already there. | |
Comment Actions
Agreed, I do plan to send patches to fix these up. I will test the warning against a wider range of code later to help evaluate how noisy it is and look for false positives.
Comment Actions
Maybe “res &= foo ();” would be better? “Changed |= foo();” is very typical for LLVM.
Comment Actions
I have sent the following patches for the Linux kernel. These were the only instances of the warning that I found across the code base in all of the configurations that we usually test so thank you for the heads up so I could send fixes before this lands.
https://lore.kernel.org/r/20210814234248.1755703-1-nathan@kernel.org/
https://lore.kernel.org/r/20210814235625.1780033-1-nathan@kernel.org/
https://lore.kernel.org/r/20210815004154.1781834-1-nathan@kernel.org/
Is there a reason -Wbool-operation is not in -Wall? It is with GCC (although now, their -Wbool-operation is equivalent to just Wbool-operation-negation. I know that is tangential to this patch but if we want to take advantage of this new warning, we will have to explicitly enable -Wbool-operation in the kernel's Makefile.
Comment Actions
-Wbool-operation is basically enabled even without -Wall, but yeah, gcc only warns with -Wall specified.
I will change it as well.
Comment Actions
Can we either emit that as a suggestion if the code looks like a = a & foo()? Or simply not emit a warning?
Comment Actions
Can we either emit that [a &= foo()] as a suggestion if the code looks like a = a & foo()? Or simply not emit a warning?
The problem is that if a is boolean, then a = a & something is 99% for sure a typo — 99% of the time, the programmer meant a = a && something. So any compiler suggestion or fixit that blindly "doubles down" on the non-short-circuiting behavior is a bad idea. https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ is relevant.
One proper way to silence the warning without short-circuiting would be a = int(a) & something — this indicates that you really do want integer bitwise-ANDing, not boolean. Alternatively, and probably best, just declare a as an integer variable to begin with. Of course the compiler cannot recommend this if something isn't strictly zero or one. But in that case you might already have a bug:
int error_returning_func() { return 2; }
int main() {
bool t = true;
t = t && error_returning_func();
assert(t); // still true
t = t & error_returning_func();
assert(!t); // now it's false
}So typo'ing & for && doesn't just turn off short-circuiting — it can also mess up the runtime value stored in the bool.
Comment Actions
The remaining places where this fired looked like places where && would be better than &, except for one where the code was treating bools as one bit integers and doing various bitwise operations on them
https://source.chromium.org/chromium/chromium/src/+/main:third_party/distributed_point_functions/src/dpf/distributed_point_function.cc;drc=87b84b3834343e141ec94e3321f4d1c7be8a7a9d;l=230
Comment Actions
This patch seems like a great contribution! Really glad to see this being added. I did have a question though on why this only appears to catch "&" vs "&&" instead of doing the same for "|" vs "||". It seems like both operators have roughly the same potential for confusion. Could we add support for bitwise vs logical or in this?
Comment Actions
Yeah, I mentioned in the first comment as part of “open questions”. Basically at first we need to find out some reasonable “heuristics” when to warn and then support for | should be added.
Comment Actions
- Only warn when both sides have potentional side effects (conversative, but covers motivating case, reduces useless noise - which may hide real bug - caused by this warning)
- Added support for bitwise | + tests.
Comment Actions
I am still running a series of builds against the Linux kernel but I already see one instance of this warning where the suggestion would change the meaning of the code in an incorrect way:
drivers/input/touchscreen.c:81:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/input/touchscreen.c:81:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/input/touchscreen.c:94:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/input/touchscreen.c:94:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/input/touchscreen.c:108:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
data_present = touchscreen_get_prop_u32(dev,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
5 warnings generated.Which corresponds to this file: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen.c?h=v5.14-rc6
If the calls to touchscreen_get_prop_u32 short circuit, we could use maximum or fuzz uninitialized. There might be a cleaner way to rewrite that block to avoid the warning but based on the other instances of this warning I see, I am not sure | vs. || is worth warning about (but I am happy to hear examples of how it could be a bug). Most people realize && short circuits (as if (a && foo(a->...)) is relatively common) but most probably are not thinking about || short circuiting (and it would be more of an optimization than correctness issue as far as I understand it).
Additionally, I have not caught any instances of & being used instead of &&, including the ones I notated previously; those were caught because only the right side had side effects. As was pointed out here and on the mailing list, the lib/zstd/ warning is probably a bug, as the short circuit should happen if offset_1 is zero, otherwise there is unnecessary work done.
Comment Actions
and it would be more of an optimization than correctness issue as far as I understand
Yeah, this is right, indeed.
Maybe @rpbeltran has some idea or motivating cases for OR pattern?
Comment Actions
I don't actually have a case off-hand that I've spotted from the real world, but this example snippet seems plausible enough:
if (queue.empty() || queue.pop() == STOP_TOKEN) {
break;
}Consider that in this scenario, we only consider queue.pop() if the queue is not empty due to short circuiting.
if (queue.empty() | queue.pop() == STOP_TOKEN) {
break;
}While this scenario calls pop() regardless, likely causing a runtime error if the queue is empty.
I'll run this warning against ChromeOS and see if I spot any interesting results.
Comment Actions
I'll run this warning against ChromeOS and see if I spot any interesting results.
Thanks!
Comment Actions
FWIW, I still see no reason not to warn on |-for-||. Again per https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ : if we see the programmer writing somebool | foo, we know they've messed up — they might have meant either int(somebool) | foo or somebool || foo, we're not sure which, but we know they've messed up somehow. So it makes sense to tell them about it. The codepaths for &&/& and ||/| are going to be shared, right? So I see no reason to special-case-not-warn on |.
I do agree that it seems reasonable for people to screw up & more frequently than |. It's very common in C and C++ to type just a single & (e.g. for address-of); it's very uncommon to ever type a single |, so the muscle memory won't be there to typo it. Also, | is way off on the side of the keyboard where the typist is probably paying a little more attention; & is right in the hot path where our fingers are likely moving quickly. But from the compiler's POV, we might be surprised to see the rare |-for-|| typo ("whoa! I hardly ever see one of those!") but that's no reason to keep it a secret from the user.
| clang/test/Sema/warn-bitwise-or-bool.c | ||
|---|---|---|
| 39 | Arguably, foo() bitor bar() is a sufficiently high "degree of ornamentation" to unambiguously signal the programmer's intent here. What are the chances that the programmer wrote bitor instead of or by accident? But it's not worth adding any code just to special-case-not-warn here. | |
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 7434–7436 |
I think we're all in agreement here. Putting this behind a separate -Wbitwise-instead-of-logical flag sounds good to me. | |
| clang/test/Sema/warn-bitwise-and-bool.c | ||
| 22–23 | I don't think a non-volatile pointer deref actually has any side effect. I think what we'd want to check here is if the expression could be reordered with the LHS. But I don't know how we'd check that. | |
Comment Actions
Sorry for the late reply on this. I ran the warning against ChromeOS and actually found very few false positives and a couple interesting findings to file bugs for. I actually saw about an equal number of cases caught for & and |.
Comment Actions
LGTM % comments, FWIW.
| clang/test/Sema/warn-bitwise-and-bool.c | ||
|---|---|---|
| 27 | Perhaps add a TODO FIXME comment here? I still think this should be fixed, but I'm willing to believe that it's unduly difficult and defer it to later. :) | |
| 28 | For each warning case (or at least for one or two of them), could we also test (my suggested) suppression mechanism? I think it "obviously" just works, but it'd be good to test explicitly that it works. b = foo() & (int)(*q == 42); // OK, no warning expected | |
Comment Actions
Maybe we should emit note like “insert explicit cast of one or both operands to int to silence this warning” (any idea for better note text?)? I think it could be useful.
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 69 | LGTM, based on the precedent set on line 68. If I were writing all these notes from scratch, I'd say "to silence this warning"; but that can be done in a separate PR, if ever. | |
| clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
|---|---|---|
| 69 | Yeah, silence this warning is better and also used (well, more often) in this file. I will change it to this form. | |
Comment Actions
@rpbeltran @nathanchance @aaron.ballman Are you OK with the current state of this patch? Well, it is clear that some code in linux kernel/Chromium/etc needs to adjusted, but I think in many cases it would (atleast) improve readability.
Comment Actions
As far as I am aware, this iteration of the patch has no instances in the Linux kernel now. The instances I found with the first iteration of the patch only had a right hand side with side effects, not both sides, so this warning is effectively a no-op now (not that there won't be instances in the future). If any happen to show up, I will send fixes for them at that time. In other words, LGTM.
Comment Actions
It found a few issues on Firefox:
https://bugzilla.mozilla.org/show_bug.cgi?id=1734285
I think it should be added it in the release notes:
https://reviews.llvm.org/D111215
Comment Actions
Thanks, all found cases are not “security/memory” issues but they should use logical op to improve code readability (no reason for bitwise op).
Comment Actions
Thanks for this warning! I just finished turning it on in Chromium. Here's a short report on the things it fired on in case it's interesting to anyone. (I'm not suggesting any changes to the warning, I think it works very well as-is).
The warning fired in 21 different files. In 16 cases, using && instead of & and || instead of | was possible, but the change was inconsequential [1]. In 6 cases, & and | were used intentionally to prevent short-circuiting, but it wasn't always obvious that that was the case. I usually rewrote the code to make it more obvious [2]. In addition to these, there was of course the true positive that motivated this warning :)
Apparently we build a (small) subset of LLVM 10 as part of some dependency of chrome. The warning fired a few times there, so I turned it off for those files (in trunk LLVM it's fixed). I also found a quality-of-implementation bug with the warning which I reported as https://bugs.llvm.org/show_bug.cgi?id=52226
1:
https://chromium-review.googlesource.com/c/angle/angle/+/3212889/2/src/libANGLE/renderer/gl/VertexArrayGL.cpp
https://chromium-review.googlesource.com/c/angle/angle/+/3212889/2/src/tests/gl_tests/CopyTexImageTest.cpp
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/content/browser/payments/payment_app_database.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/gin/v8_initializer.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/net/spdy/spdy_http_stream.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/services/network/url_loader.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3229610/2/chromecast/cast_core/url_rewrite_rules_adapter.cc
https://chromium-review.googlesource.com/c/v8/v8/+/3212891/2/src/objects/feedback-vector.cc
https://chromium-review.googlesource.com/c/v8/v8/+/3231077/2/src/codegen/arm64/assembler-arm64.cc
https://chromium.googlesource.com/external/github.com/google/distributed_point_functions/+/61ff31aecab6955ad9ed2dac8684375f999eb9f8%5E%21/#F0
https://dawn-review.googlesource.com/c/dawn/+/66200/2/src/tests/end2end/QueryTests.cpp
https://github.com/KhronosGroup/Vulkan-ValidationLayers/commit/51279399eaecf651d176544d8197f91f48637b2f
https://github.com/netwide-assembler/nasm/pull/17
https://skia-review.googlesource.com/c/skia/+/459456/3/src/core/SkColorFilter.cpp
https://webrtc-review.googlesource.com/c/src/+/234600/2/media/engine/simulcast_encoder_adapter.cc
2:
https://bugs.chromium.org/p/chromium/issues/detail?id=1261591 (still undecided)
https://chromium-review.googlesource.com/c/v8/v8/+/3212891/2/src/compiler/branch-elimination.cc
https://github.com/harfbuzz/harfbuzz/pull/3256
https://skia-review.googlesource.com/c/skia/+/459456/3/modules/skottie/src/animator/Vec2KeyframeAnimator.cpp
https://skia-review.googlesource.com/c/skia/+/459456/3/modules/skottie/src/animator/VectorKeyframeAnimator.cpp
https://skia-review.googlesource.com/c/skia/+/459456/3/src/core/SkPathEffect.cpp
LGTM, based on the precedent set on line 68. If I were writing all these notes from scratch, I'd say "to silence this warning"; but that can be done in a separate PR, if ever.