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
Paths
| Differential D108003
[Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects ClosedPublic Authored by xbolva00 on Aug 12 2021, 3:08 PM.
Details Summary 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
Diff Detail
Unit TestsFailed
Event TimelineComment Actions Open questions: @hans @nickdesaulniers maybe you want to try this new warning on Chrome/Linux kernel? xbolva00 retitled this revision from [Clang] Extend -Wbool-operation to warn for bitwise and of bools with side effects to [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects.Aug 12 2021, 3:13 PM • Quuxplusone added inline comments.
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.
xbolva00 added inline comments. 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). 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.
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/ 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
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 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
Comment Actions
New revision should be more conversative to avoid warning on cases you mentioned. 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
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
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 |.
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.
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.
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. Closed by commit rGf62d18ff140f: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side… (authored by xbolva00). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions It found a few issues on Firefox: I think it should be added it in the release notes: 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: 2:
Revision Contents
Diff 375969 clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaChecking.cpp
clang/p.patch
clang/test/Sema/warn-bitwise-and-bool.c
clang/test/Sema/warn-bitwise-or-bool.c
|
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.