Add attribute for return values that shouldn't be cast to bool

[urusant]

Summary

Hi,

I am interested in feedback on a patch I was working on that adds a new attribute (warn_impcast_to_bool) to indicate that the return value of a function shouldn't be used as a boolean, as well as a compile warning and a StaticAnalyzer checker to warn about misusing functions with this attribute. I'd also appreciate any suggestions for how to deal with a class of false positives that the static analysis checker produces.

The change was originally inspired by CVE-2008-5077 [1], which was the result of an odd design choice in OpenSSL: having an API that returns 1 for success, 0 for failure... and -1 for catastrophic failure. Various API users fell into the trap of treating the return value as a boolean, so the patch adds an attribute to allow this to trigger a warning.

As well as generating a compile-time warning, the patch also includes a new static analyzer checker to catch more indirect uses, where the non-boolean integer value gets propagated via function wrappers or local variables. However, it gives a false positive for cases when the code using the return value actually checks the value in a non-boolean way (because the SVal doesn't reflect the fact that the value has been further constrained). I couldn't see an obvious way to get anything relevant from the RangeConstraintManager; any suggestions?

To test the check (beyond the included unit tests), I annotated dangerous OpenSSL functions and tried building 8 OpenSSL-using codebases with it. So far, this didn't give many results for them: the only possible problem was found in ruby2.1, which was already fixed a few months ago. However, this change is still potentially useful - even 7 years after the original CVE, there are still codebases that fall into OpenSSL's API trap.

[1] https://www.openssl.org/news/secadv/20090107.txt

[aaron.ballman]

Thank you for working on this check! A few comments:

The patch is missing Sema tests for the attribute (that it only applies to declarations you expect, accepts no args, etc).

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration? Right now, the diagnostic you receive on a conversion may be spatially separated from where the user called the function (including crossing translation unit boundaries, which the static analyzer doesn't currently handle). By putting the attribute on the type, it carries more obvious semantic meaning and means the check can happen entirely in the frontend (no static analyzer required). For instance: typedef __attribute__((warn_impcast_to_bool)) IntNotABool; I'm not certain if this is a better design or not, but I am wondering if it was something you had considered.

include/clang/Basic/Attr.td

1138

This should not use a GCC spelling because it's not an attribute that GCC supports. You should probably use GNU instead, since I suspect this attribute will be useful in C as well as C++.

1140

No need to specify the WarnDiag or ExpectedFunctionOrMethod arguments; they will be handled automatically.

include/clang/Basic/AttrDocs.td

2058

You should manually wrap this to roughly the 80 col limit.

Instead of "he", can you use "they" please?

include/clang/Basic/DiagnosticGroups.td

57 ↗

(On Diff #71157)

I'm not certain this requires its own diagnostic group. This can probably be handled under BoolConversion

include/clang/Basic/DiagnosticSemaKinds.td

2259

How about: ...only applies to integer return types?

2883

I don't think this should be a DefaultIgnore diagnostic -- if the user wrote the attribute, they should get the diagnostic when appropriate.

lib/Sema/SemaChecking.cpp

8262

Should use if (const auto *CE = dyn_cast<CallExpr>(E)) {

8263–8264

Then you can do if (const auto *Fn = CE->getDirectCallee()) {

8269

You can pass in fn directly, the diagnostics engine will properly get the name out of it because it's derived from NamedDecl.

lib/Sema/SemaDeclAttr.cpp

1316

Formatting seems off -- you should run the patch through clang-format. Also, why are you passing an empty SourceRange?

test/ReturnNonBoolTestCompileTime.cpp

40 ↗

(On Diff #71157)

Can you end the file with a newline?

[zaks.anna]

include/clang/Basic/AttrDocs.td

2055

You probably need to "propose" the attribute to the clang community. I'd send an email to the cfe-dev as it might not have enough attention if it's just the patch.

test/ReturnNonBoolTest.c

74 ↗

(On Diff #71157)

I do not understand why this is a false positive. In restricted_wrap, r can be any value. You only return '0' if r is '-1', but it could be '-2' or '100', which are also not bool and this values would just get returned.

You should be able to query the state to check if a value is a zero or one using code like this from CStringChecker.cpp:
"

SValBuilder &svalBuilder = C.getSValBuilder();
DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
return state->assume(svalBuilder.evalEQ(state, *val, zero))

"

[urusant]

Made some changes based on the comments. Please refer to the replies below.

[urusant]

Thank you for the feedback.

The patch is missing Sema tests for the attribute (that it only applies to declarations you expect, accepts no args, etc).

There is one test case for that in test/ReturnNonBoolTestCompileTime.cpp. I've added another one for attribute accepting no args, so now the last two test cases in this file are those you were asking about. Can you think of any other cases of invalid attribute usage?

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?

No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like int r = X509_verify_cert(...) and the function X509_verify_cert has a return type with attribute, r won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?

include/clang/Basic/Attr.td

1138

Yeah, makes sense.

1140

I didn't know that, thanks.

include/clang/Basic/AttrDocs.td

2055

OK, will do.

2058

OK, I did that. However, 80 col limit in this case feels a bit inconsistent with the rest of the file to me because most of other similar descriptions don't follow it.

include/clang/Basic/DiagnosticGroups.td

57 ↗

(On Diff #71157)

OK.

include/clang/Basic/DiagnosticSemaKinds.td

2259

Yeah, that sounds better.

2883

Makes sense.

lib/Sema/SemaChecking.cpp

8262

Done.

8263–8264

Done.

8269

Thanks, didn't notice that.

lib/Sema/SemaDeclAttr.cpp

1316

Ok, I ran clang-format.
Good spot, it seems that I don't need that SourceRange.

test/ReturnNonBoolTest.c

74 ↗

(On Diff #71157)

I have replaced this test case with another one that illustrates the problem I am referring to clearer. Ideally it would be great to have some indicator to tell the StaticAnalyzer that we have handled all the dangerous return values, and from this point it is safe to use it as a boolean. You can use explicit cast to bool or rc != 0 every time you want to use it, but it is not very convenient. Do you have any suggestions on this matter?

As for your proposal, it is not very difficult to add, however, it is not very likely to be useful in real codebases for the same reason as in the testcase. Do you still think it should be added?

test/ReturnNonBoolTestCompileTime.cpp

40 ↗

(On Diff #71157)

Done.

[aaron.ballman]

In D24507#546241, @urusant wrote:

Thank you for the feedback.

The patch is missing Sema tests for the attribute (that it only applies to declarations you expect, accepts no args, etc).

There is one test case for that in test/ReturnNonBoolTestCompileTime.cpp. I've added another one for attribute accepting no args, so now the last two test cases in this file are those you were asking about. Can you think of any other cases of invalid attribute usage?

We try to keep our tests segregated by functionality. e.g., tests relating to the way the attribute is handled (what it appertains to, args, etc) should live in Sema, tests relating to the static analyzer behavior should live in test/Analysis, etc.

Tests that are still missing are: applying to a non-function type, applying to a member function, applying to an Obj-C method. For member functions, what should happen if the function is virtual? What if the overriders do not specify the attribute? What if an override specifies the attribute but the base does not?

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?

No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like int r = X509_verify_cert(...) and the function X509_verify_cert has a return type with attribute, r won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?

I was thinking it would be diagnosed if you attempted to assign from your attributed type to a type that is not compatible. However, that may still be problematic because it raises other questions (can you SFINAE on it? Overload? etc).

[urusant]

In D24507#546380, @aaron.ballman wrote:

We try to keep our tests segregated by functionality. e.g., tests relating to the way the attribute is handled (what it appertains to, args, etc) should live in Sema, tests relating to the static analyzer behavior should live in test/Analysis, etc.

Tests that are still missing are: applying to a non-function type, applying to a member function, applying to an Obj-C method. For member functions, what should happen if the function is virtual? What if the overriders do not specify the attribute? What if an override specifies the attribute but the base does not?

I have added the test cases about member functions.
As for ObjC methods, I didn't pay much attention to them while developing the check as ObjC wasn't the primary target. I tried to make a test case for it, and it turned out that it is OK to put an attribute on ObjC method, but you wouldn't get neither compiler warning nor StaticAnalyzer report. That is why I removed ObjC methods from the attribute subjects and replaced the ObjC test case with another one that shows that you cannot apply the attribute to ObjC methods (not sure if it is still necessary, because it seems not very different from applying the attribute to a non-function variable - in both cases we get the same warning). Do you think it's worth digging into how to make it work with ObjC? In this case I might need some help because I don't really speak Objective C.

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration?

No, I hadn't. On a quick look though, I couldn't find a way to simplify my solution using this idea, because as far as I understand, the type attribute isn't inherited, so, for example, if I have something like int r = X509_verify_cert(...) and the function X509_verify_cert has a return type with attribute, r won't have the attribute. If that is correct, we still need to backtrace the value to the function declaration. Is there something I am missing?

I was thinking it would be diagnosed if you attempted to assign from your attributed type to a type that is not compatible. However, that may still be problematic because it raises other questions (can you SFINAE on it? Overload? etc).

This might also make the check itself easier (as we don't need path-sensitive analysis), however, it would make the use more complicated as all the users of the dangerous function would have to change their code (even if they are using it correctly). For example, if we refer to the original motivation, annotating dangerous OpenSSL functions would allow us to protect dozens of codebases using them without changing every one of them.

[danielmarjamaki]

include/clang/Basic/AttrDocs.td

2055

I saw your email on cfe-dev. This sounds like a good idea to me.

lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp

50

It seems you need to run clang-format on this file also.

test/ReturnNonBoolTest.c

7 ↗

(On Diff #71807)

sorry but why do you have a #ifdef clang isn't it always defined?

[urusant]

lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp

51

I have just noticed that I didn't specify the style option when I ran it the first time. Now it should be fine.

test/ReturnNonBoolTest.c

7 ↗

(On Diff #71807)

If I were to add the attribute to a function in some real codebase, I would probably want to save different compilers compatibility. However, it might not be necessary for the testcases.

[zaks.anna]

test/Analysis/ReturnNonBoolTest.c

67

How about addressing this as follows: in checkBranchCondition, you check for any comparisons of the tracked value other than comparisons to bool. If you see such a comparison, you assume that the error handling has occurred and remove the symbol from the set of tracked symbols. This will ensure that any code after the cleansing condition (error handling) can cast the return value to bool.

The warning will still get triggered if the error handling is after the comparison to bool. That could be avoided as well, but the solution would be more complicated. I am thinking something along the lines of tracking all comparisons until the symbol goes out of scope. For each symbol, you'd track it's state (for example, "performedErrorHandling | comparedToBoolAndNoErrorHandling | notSeen"). You can draw the automaton to see what the transitions should be. When the symbol goes out of scope, you'd check if it's state is "comparedToBoolAndNoErrorHandling". Further, we'd need to walk up the path to find the location where we compared the symbol and use that for error reporting.

include/clang/Basic/Attr.td

+// An attribute indicating that a function/method return value is not safe to be
+// treated as bool.
+def WarnImpcastToBool : InheritableAttr {
+  let Spellings = [GNU<"warn_impcast_to_bool">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [WarnImpcastToBoolDocs];
+}
+

include/clang/Basic/AttrDocs.td

+def WarnImpcastToBoolDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+    The ``warn_impcast_to_bool`` attribute is used to indicate that the return
+    value of a function with integral return type cannot be used as a boolean
+    value. For example, if a function returns -1 if it couldn't efficiently read
+    the data, 0 if the data is invalid and 1 for success, it might be dangerous
+    to implicitly cast the return value to bool, e.g. to indicate success.
+    Therefore, it is a good idea to trigger a warning about such cases. However,
+    in case a programmer uses an explicit cast to bool, that probably means that
+    they know what they are doing, therefore a warning should be triggered only
+    for implicit casts.
+
+    .. code-block:: c
+
+    int f(int x) __attribute__((warn_impcast_to_bool));
+
+    void test(int x) {
+      if (f(x)) { // diagnoses
+      }
+      if ((bool)f(x)) { // Does not diagnose, explicit cast.
+      }
+    }
+  }];
+}

include/clang/Basic/DiagnosticSemaKinds.td

+def warn_attribute_return_int_only : Warning<
+  "%0 attribute only applies to integer return types">,
+  InGroup<IgnoredAttributes>;
+
+def warn_impcast_non_bool_to_bool : Warning<
+  "implicit conversion turns non-bool into bool: %0 to %1">,
+  InGroup<BoolConversion>;
+

include/clang/StaticAnalyzer/Checkers/Checkers.td

+def ReturnNonBoolChecker : Checker<"ReturnNonBool">,
+  HelpText<"Check for dangerous conversion of integral return values to bool.">,
+  DescFile<"ReturnNonBoolChecker.cpp">;

lib/Sema/SemaChecking.cpp

+    /// Warn if the expression is the return value of a function call being
+    /// implicitly cast to bool, while it's specified that it shouldn't be by a
+    /// 'warn_impcast_to_bool' attribute.
+    ///
+    /// Note that this isn't triggered if the function call is part of a more
+    /// complicated expression, which in turn is cast to bool,
+    /// e.g. (x ? f : g)(y)
+    if (const auto *CE = dyn_cast<CallExpr>(E)) {
+      if (const auto Fn = CE->getDirectCallee()) {
+        if (Fn->hasAttr<WarnImpcastToBoolAttr>()) {
+          DiagnoseImpCast(S, E, T, CC, diag::warn_impcast_non_bool_to_bool);
+          S.Diag(Fn->getLocation(), diag::note_entity_declared_at) << Fn;
+          return;
+        }
+      }
+    }

lib/Sema/SemaDeclAttr.cpp

+static void handleWarnImpcastToBoolAttr(Sema &S, Decl *D,
+                                        const AttributeList &Attr) {
+  QualType ResultType = getFunctionOrMethodResultType(D);
+  SourceRange SR = getFunctionOrMethodResultSourceRange(D);
+  if (!ResultType->isIntegralOrEnumerationType()) {
+    S.Diag(Attr.getLoc(), diag::warn_attribute_return_int_only)
+        << Attr.getName() << SR;
+    return;
+  }
+
+  D->addAttr(::new (S.Context) WarnImpcastToBoolAttr(
+      Attr.getRange(), S.Context, Attr.getAttributeSpellingListIndex()));
+}
+
+  case AttributeList::AT_WarnImpcastToBool:
+    handleWarnImpcastToBoolAttr(S, D, Attr);
+    break;

lib/StaticAnalyzer/Checkers/CMakeLists.txt

+  ReturnNonBoolChecker.cpp

lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp

+//=== ReturnNonBoolChecker.cpp - Non-boolean returns checker ----*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines ReturnNonBoolChecker that warns about dangerous conversions of
+// integral return values to bool. Such impcast is considered dangerous if this
+// is specified by a warn_impcast_to_bool attribute.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/AST/ParentMap.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class ReturnNonBoolChecker
+    : public Checker<check::BranchCondition, check::PreStmt<ImplicitCastExpr>,
+                     check::PreStmt<UnaryOperator>, check::PostCall> {
+  mutable std::unique_ptr<BuiltinBug> BT_impcast;
+  void checkExprValue(CheckerContext &C, const Expr *E) const;
+
+public:
+  void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const;
+  void checkPreStmt(const ImplicitCastExpr *ICE, CheckerContext &C) const;
+  void checkPreStmt(const UnaryOperator *UO, CheckerContext &C) const;
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+};
+} // end anonymous namespace
+
+// A set of values specified to be dangerous for conversion to bool.
+REGISTER_SET_WITH_PROGRAMSTATE(NonBoolValues, SymbolRef)
+
+void ReturnNonBoolChecker::checkExprValue(CheckerContext &C,
+                                          const Expr *E) const {
+  ProgramStateRef State = C.getState();
+  SymbolRef SR = State->getSVal(E, C.getLocationContext()).getAsSymbol();
+  // If the value isn't marked as dangerous to be cast to bool, no warning is
+  // needed.
+  if (!State->contains<NonBoolValues>(SR))
+    return;
+
+  ExplodedNode *N = C.generateErrorNode(C.getState());
+  if (!N)
+    return;
+  if (!BT_impcast)
+    BT_impcast.reset(new BuiltinBug(
+        this, "implicit cast to bool is dangerous for this value"));
+  C.emitReport(llvm::make_unique<BugReport>(*BT_impcast,
+                                            BT_impcast->getDescription(), N));
+}
+
+// This catches 'conversion to bool'-like cases in C, where there is no boolean
+// type or implicit cast to bool.
+void ReturnNonBoolChecker::checkBranchCondition(const Stmt *Condition,
+                                                CheckerContext &C) const {
+  const Expr *E = dyn_cast<Expr>(Condition);
+  if (!E)
+    return;
+  const Type *TypePtr = E->getType().getCanonicalType().getTypePtr();
+  // If the expression is boolean, then either it is fine to use it or it is
+  // a cast expression. If it is an explicit cast, it is fine because this means
+  // that author knows what he is doing, otherwise it will be caught by
+  // checkPreStmt<ImplicitCastExpr>, so we don't need to do anything here.
+  if (!TypePtr->isSpecificBuiltinType(BuiltinType::Bool))
+    checkExprValue(C, E);
+}
+
+// Checks if the parent of the specified implicit cast in the AST is a CastExpr.
+static bool hasParentCast(const ImplicitCastExpr *ICE,
+                          const LocationContext *LC) {
+  const Stmt *Parent = LC->getParentMap().getParent(ICE);
+  return Parent && dyn_cast<CastExpr>(Parent);
+}
+
+// Checks if the given cast expression is an integral to boolean cast.
+static bool isIntToBoolCast(const CastExpr *CE) {
+  const Type *TypePtr = CE->getType().getCanonicalType().getTypePtr();
+  const Type *OrigTypePtr =
+      CE->getSubExpr()->getType().getCanonicalType().getTypePtr();
+  return OrigTypePtr->isIntegralOrEnumerationType() &&
+         TypePtr->isSpecificBuiltinType(BuiltinType::Bool);
+}
+
+// This catches implicit casts from integral types to bool in case they exist,
+// i.e. for C++.
+void ReturnNonBoolChecker::checkPreStmt(const ImplicitCastExpr *ICE,
+                                        CheckerContext &C) const {
+  // Some types of casts (e.g. C-style cast) have implicit cast as a child,
+  // which we don't really care about because in this case this is actually an
+  // explicit cast, which means that the programmer is aware that it's
+  // dangerous, but still wants to do it.
+  if (hasParentCast(ICE, C.getLocationContext()))
+    return;
+  // This checker is only for integral to boolean casts.
+  if (!isIntToBoolCast(dyn_cast<CastExpr>(ICE)))
+    return;
+  checkExprValue(C, ICE->getSubExpr());
+}
+
+// This is to catch logical negotiation operator, which is an int->int operator
+// in C, so it is not caught by either of the two previous methods.
+void ReturnNonBoolChecker::checkPreStmt(const UnaryOperator *UO,
+                                        CheckerContext &C) const {
+  if (UO->getOpcode() != UO_LNot)
+    return;
+  const Expr *E = UO->getSubExpr();
+  const Type *TypePtr = E->getType().getCanonicalType().getTypePtr();
+  if (!TypePtr->isIntegralOrEnumerationType())
+    return;
+  checkExprValue(C, E);
+}
+
+// Store the symbolic reference for return value of "ReturnsNonBool" function
+// in the set.
+void ReturnNonBoolChecker::checkPostCall(const CallEvent &Call,
+                                         CheckerContext &C) const {
+  const Decl *Function = Call.getDecl();
+  if (!Function || !Function->hasAttr<WarnImpcastToBoolAttr>())
+    return;
+  SymbolRef ReturnValue = Call.getReturnValue().getAsSymbol();
+  // If the return value cannot be taken as symbol, we don't want to add it
+  // to the set because it can be achieved by many different ways, and we don't
+  // want them to be treated as equals.
+  if (!ReturnValue)
+    return;
+  ProgramStateRef State = C.getState();
+  State = State->add<NonBoolValues>(ReturnValue);
+  C.addTransition(State);
+  return;
+}
+
+void ento::registerReturnNonBoolChecker(CheckerManager &mgr) {
+  mgr.registerChecker<ReturnNonBoolChecker>();
+}

test/Analysis/ReturnNonBoolTest.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.core.ReturnNonBool -Wno-bool-conversion -verify %s
+
+/// C is checked slightly differently than C++, in particular, C doesn't have
+/// implicit casts to bool, so we need to test different branching situations,
+/// like if, for, while, etc.
+
+#ifdef __clang__
+#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool))
+#else
+#define RETURNS_NON_BOOL
+#endif
+
+int NonBool(int x) RETURNS_NON_BOOL;
+
+void test_if() {
+  if (NonBool(2))  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+}
+
+void test_while() {
+  while (NonBool(2))  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    continue;
+}
+
+void test_for() {
+  for (; NonBool(2);)  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    continue;
+}
+
+void test_and(int x, int y) {
+  if (NonBool(2) && (x == y))  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+}
+
+void test_or() {
+  if (NonBool(2) || (1 != 1))  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+}
+
+void test_not() {
+  if (!NonBool(2))  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+}
+
+int test_ternary() {
+  return NonBool(2) ? 1 : 0;  // expected-warning{{implicit cast to bool is dangerous for this value}}
+}
+
+int wrap(int x) {
+  int r = NonBool(x);
+  return r;
+}
+
+void test_wrap() {
+  if (wrap(2))  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+}
+
+// Example inspired by CVE-2008-5077:
+// Returns 1 on success, 0 on failure and something negative on catastrophic
+// failure
+int verify_cert() __attribute__((warn_impcast_to_bool));
+
+void correctly_handled() {
+  int rc = verify_cert();
+
+  if (rc < 0)
+    // error handling
+
+  if (rc) { // expected-warning{{implicit cast to bool is dangerous for this value}}
+    // Here we unfortunately get a warning although the code does correctly
+    // handle the documented return codes. However, the static analysis checker
+    // can't read the comment (or the manpage)...
+  }
+  // However, the warning can be easily suppressed, for example, like this:
+  if (rc != 0) {
+  }
+  // In C++ you can use explicit cast to bool as well.
+}

test/Analysis/ReturnNonBoolTest.cpp

+// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=alpha.core.ReturnNonBool -Wno-bool-conversion -verify %s
+#ifdef __clang__
+#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool))
+#else
+#define RETURNS_NON_BOOL
+#endif
+
+int NoAttributes() { return 2; }
+
+int NonBool(int x) RETURNS_NON_BOOL;
+
+int good(int x);
+
+int wrap(int x) {
+  int r = NonBool(x);
+  return r;
+}
+
+void test1() {
+  if (NonBool(1)) {  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+  }
+}
+
+void test2() {
+  if (wrap(2)) {  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+  }
+}
+
+void test3() {
+  if ((bool)NonBool(3)) {  // no warning, explicit cast
+    return;
+  }
+}
+
+void test4(int x) {
+  if (bool(wrap(2 * x))) {  // no warning, explicit cast
+    return;
+  }
+}
+
+void test5() {
+  if (good(5)) {  // no warning, return value isn't marked as dangerous
+    return;
+  }
+}
+
+void test6() {
+  if (good(wrap(2))) {  // no warning, wrap is treated as int, not as bool
+    return;
+  }
+}
+
+double InvalidAttributeUsage()
+    RETURNS_NON_BOOL;  // expected-warning{{'warn_impcast_to_bool' attribute only applies to integer return types}}
+
+void test_function_pointer(void (*f)()) {
+  // This is to test the case when Call.getDecl() returns NULL, because f()
+  // doesn't have a declaration
+  f();
+}
+
+bool universal_bool_wrapper(int (*f)(int), int x) {
+  // When we call universal_bool_wrapper from test_universal_bool_wrapper, the
+  // analyzer follows the path and detects that in this line we are doing
+  // something wrong (assuming that f is actually NonBool). So if we didn't call
+  // universal_bool_wrapper with any dangerous function, there would be no
+  // warning.
+  return f(x);  // expected-warning {{implicit cast to bool is dangerous for this value}}
+}
+
+int universal_int_wrapper(int (*f)(int), int x) { return f(x); }
+
+void test_universal_bool_wrapper(int x) {
+  if (universal_bool_wrapper(NonBool, x)) return;
+}
+
+void test_universal_int_wrapper(int x) {
+  if (universal_int_wrapper(NonBool, x))  // expected-warning{{implicit cast to bool is dangerous for this value}}
+    return;
+}
+
+void test_lambdas(int x) {
+  if ([](int a) __attribute__((warn_impcast_to_bool))-> int{ return a; }(x)) { // expected-warning{{implicit cast to bool is dangerous for this value}}
+  }
+}
+

test/SemaCXX/ReturnNonBoolTestCompileTime.cpp

+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wbool-conversion -verify %s
+#ifdef __clang__
+#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool))
+#else
+#define RETURNS_NON_BOOL
+#endif
+
+int NoAttributes() { return 2; }
+
+int NonBool(int x) RETURNS_NON_BOOL;
+int NonBool(int x) {  // expected-note{{'NonBool' declared here}}
+  return x * 2;
+}
+
+int good(int x) { return x * 2; }
+
+void test1() {
+  if (NonBool(2)) {  // expected-warning{{implicit conversion turns non-bool into bool: 'int' to 'bool'}}
+    return;
+  }
+}
+
+void test3() {
+  if ((bool)NonBool(2)) {  // no warning, explicit cast
+    return;
+  }
+}
+
+void test5() {
+  if (good(2)) {  // no warning, return value isn't marked as dangerous
+    return;
+  }
+}
+
+double InvalidReturnType() RETURNS_NON_BOOL;  // expected-warning{{'warn_impcast_to_bool' attribute only applies to integer return types}}
+
+int AttributeWithArguments() __attribute__((warn_impcast_to_bool(2)));  // expected-error {{'warn_impcast_to_bool' attribute takes no arguments}}
+
+int NonFunction RETURNS_NON_BOOL; // expected-warning{{'warn_impcast_to_bool' attribute only applies to functions}}
+
+class Base {
+  int data;
+ public:
+  virtual int SafeMember();
+  virtual int NonBoolMember() RETURNS_NON_BOOL;
+};
+
+class DerivedCorrect : public Base {
+ public:
+  int SafeMember();
+  int NonBoolMember() RETURNS_NON_BOOL; // expected-note{{'NonBoolMember' declared here}}
+};
+
+class DerivedIncorrect : public Base {
+ public:
+  int SafeMember() RETURNS_NON_BOOL; // expected-note {{'SafeMember' declared here}}
+  int NonBoolMember();
+};
+
+void TestMemberFunctions() {
+  DerivedCorrect DC;
+  DerivedIncorrect DI;
+  if (DC.SafeMember()) {}
+  if (DI.SafeMember()) {} // expected-warning{{implicit conversion turns non-bool into bool: 'int' to 'bool'}}
+  if (DC.NonBoolMember()) {} // expected-warning{{implicit conversion turns non-bool into bool: 'int' to 'bool'}}
+  if (DI.NonBoolMember()) {}
+}
+
+
+

test/SemaObjC/ReturnNonBoolTestCompileTime.m

+// RUN: %clang_cc1  -fsyntax-only -verify %s
+
+@interface INTF
+- (int) fee __attribute__((warn_impcast_to_bool)); // expected-warning{{'warn_impcast_to_bool' attribute only applies to functions}}
++ (int) c __attribute__((warn_impcast_to_bool)); // expected-warning{{'warn_impcast_to_bool' attribute only applies to functions}}
+@end