Page MenuHomePhabricator

Warn on bool* to bool conversion
AbandonedPublic

Authored by hiraditya on Apr 12 2018, 4:23 PM.

Details

Summary

We found conversion from bool* to bool to be a common source of bug, so we have implemented this warning. Sample use case:

int bar(bool b) {
  return b;
}

int baz() {
  bool *b; 
  bar(b);
  return 0;
}

Typically, there would be a function which takes a bool, which gets a pointer to boolean at the call site. The compiler currently does not warn which results
in a difficult to debug runtime failure.

Diff Detail

Event Timeline

hiraditya created this revision.Apr 12 2018, 4:23 PM
smeenai added a subscriber: smeenai.

There is Clang-tidy's readability-implicit-bool-conversion check.

It may be reasonable to check pointers to integers too, since integers are often implicitly converted to bools.

Please mention new warning in Release Notes.

Quuxplusone added inline comments.
test/CXX/expr/expr.unary/expr.unary.op/p6.cpp
18 ↗(On Diff #142295)

This is not "comparing" anything, so the warning seems inappropriate here. Maybe "implicitly converting 'bool *' to 'bool' in operand of '!'` would be more appropriate?

Please add test cases for operator!, operator&&, operator||, and operator?.

test/Sema/static-init.c
10 ↗(On Diff #142295)

Again, not "comparing" anything.
Incidentally, do you know why this line fails to produce an "address of 't' will always evaluate to 'true'" warning?

test/SemaCXX/warn-bool-ptr-to-bool.cpp
6

Please add a test case

template<class T>
T foo(T *ptr) {
    return ptr ? *ptr : T{};
}
bool bar(bool *ptr) { return foo(ptr); }

and make sure the warning does not trigger in this case. (It would be OK to trigger a clang-tidy check in this case, but IMHO not a -W -Wall -Wextra warning.)

@hiraditya I personally don't like when i'm being told so, but i'd like to see some numbers...
Please run this on some big C++ project (LLVM (but you'll have to enable this diag specifically), google chrome, ???), and analyse the results.

There is Clang-tidy's readability-implicit-bool-conversion check.

It may be reasonable to check pointers to integers too, since integers are often implicitly converted to bools.

While warning on bool*->bool *might* be ok, also warning for all inttype*->bool will likely be *too* noisy for clang diagnostic.

lebedev.ri added inline comments.Apr 13 2018, 4:50 AM
include/clang/Basic/DiagnosticSemaKinds.td
7806

Also, this really really should be under it's own flag, which in turn may be under Extra.

aaron.ballman added a subscriber: aaron.ballman.

I'd also be interested to see the number of false positives and true positives when run over some large code bases (both C and C++). For instance, I would imagine code like this to be somewhat common and very reasonable:

void some_func(int someArg, bool *someResult) {
  if (someResult)
    *someResult = false;
}

Especially in C code where references are not available. This makes me wary of making this a compiler diagnostic, but clang-tidy may still be a reasonable place for this functionality to live.

...
This makes me wary of making this a compiler diagnostic, but clang-tidy may still be a reasonable place for this functionality to live.

There is Clang-tidy's readability-implicit-bool-conversion check.

I'll probably make this warning for function arguments only (when bool* is passed to a function accepting bool). Many conditionals use bool* -> bool conversion as pointed out by @Quuxplusone and @aaron.ballman

sberg added a subscriber: sberg.Apr 17 2018, 2:12 AM

A random data point from trying this patch on the LibreOffice code base:

  • a little over 100 cases that are easily identified as false positives (many of the form "if (p) *p = ...")
  • two or three cases that looked suspicious on first glance but turned out to be false positives too (but where the code apparently benefits from cleaning up)
  • two cases of false positives that are awkward to silence (via "!= nullptr") in a way compatible with older C++ standards, as they are of the form "if (bool * p = ...)", and can be rewritten to "if (bool * p = ...; p != nullptr)" only for recent C++
  • one false positive in system-provided /usr/include/qt5/QtCore/qstring.h
  • no true positives found

Note that we recently relaxed a similar diagnostic for NSNumber * in the static analyzer. Such code is semantically similar to inttype *.
https://reviews.llvm.org/D44044

thakis added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
7806

The guideline is actually "most warnings should be high-value and have next to no false positives", and because of that most warnings should be default-on. We don't have a clear guideline when to put warnings under -Wall instead of making them default-on, but sometimes "does the warning need to build a CFG?" is used for that. Almost nothing should be in Extra but not in All.

lebedev.ri added inline comments.Apr 17 2018, 5:25 AM
include/clang/Basic/DiagnosticSemaKinds.td
7806

The point i was trying to make:
if the warning is under a global umbrella flag only (All, Extra),
it can't be selectively disabled. And that is not good regardless.

hiraditya updated this revision to Diff 142776.Apr 17 2018, 7:46 AM

Warn on bool* to bool conversion during a call only.

efriedma added inline comments.
lib/Sema/SemaChecking.cpp
2623

Have you considered skipping the isBooleanType() check? Passing any pointer to a function which expects a boolean parameter seems fundamentally suspicious.

Quuxplusone added inline comments.Apr 17 2018, 10:50 AM
lib/Sema/SemaChecking.cpp
2623

We already know that LLVM itself passes pointers to functions in many places, and they're not bugs. I mean, this isn't the first time someone's proposed that pointer->bool conversions are evil. :)

I know this just came up on a mailing list somewhere recently (rsmith was involved in the rebuttal, as was I), but all I can find on Google is:
https://groups.google.com/a/isocpp.org/d/msg/std-proposals/a3g9qXFVGCs/Wj40qj48H3wJ
Anyway, the punch line I'm thinking of is something like

FooBar *FB = ...;
LLVMAssert(FB, "expected fb to be set");

but with whatever the real names are. Found a bunch of those when I wrote the diagnostic. And they're fairly solidly in the "not bugs" category.

The thing about the bool*-only version is that bool pointers are rare in C++, so I'm not sure we're gaining much. But if we can't do something more general, there's still some benefit.

I see your point about false positives for the more general version. I was sort of considering an attribute to allow implicit conversions to bool for a specific function (like an assertion), and then we could ban implicit conversions to bool for parameters to other functions. But that's probably overkill.

Quuxplusone added inline comments.Apr 17 2018, 11:23 AM
lib/Sema/SemaChecking.cpp
2623

The thing about the bool*-only version is that bool pointers are rare in C++, so I'm not sure we're gaining much. But if we can't do something more general, there's still some benefit.
I see your point about false positives for the more general version. I was sort of considering an attribute to allow implicit conversions to bool for a specific function (like an assertion), and then we could ban implicit conversions to bool for parameters to other functions. But that's probably overkill.

Based on @sberg's feedback that this warning found no true positives, and @Eugene.Zelenko's feedback that this warning already exists in clang-tidy today, personally I favor the resolution of "leave this warning in clang-tidy and abandon the attempt to move it into clang".

aaron.ballman added inline comments.Apr 17 2018, 11:27 AM
lib/Sema/SemaChecking.cpp
2623

... personally I favor the resolution of "leave this warning in clang-tidy and abandon the attempt to move it into clang".

This is my view as well.

I agree that this is one of those warnings that just can't be done in any reasonable way.

hiraditya abandoned this revision.Apr 26 2018, 9:18 AM

It appears this warning may not be always useful because there will be false positives.