Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
urusant updated this revision to Diff 71807.
urusant added a comment.
Made some changes based on the comments. Please refer to the replies below.
Repository:
rL LLVM
https://reviews.llvm.org/D24507
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDeclAttr.cpp
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
test/ReturnNonBoolTest.c
test/ReturnNonBoolTest.cpp
test/ReturnNonBoolTestCompileTime.cpp
Index: test/ReturnNonBoolTestCompileTime.cpp
===
--- /dev/null
+++ test/ReturnNonBoolTestCompileTime.cpp
@@ -0,0 +1,37 @@
+// 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}}
Index: test/ReturnNonBoolTest.cpp
===
--- /dev/null
+++ test/ReturnNonBoolTest.cpp
@@ -0,0 +1,87 @@
+// 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}}
+ }
+}
Index: test/ReturnNonBoolTest.c
===
--- /dev/null
+++ test/ReturnNonBoolTest.c
@@ -0,0 +1,79 @@
+// 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_B
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
urusant added a comment.
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?
Comment at: include/clang/Basic/Attr.td:1138
@@ +1137,3 @@
+def WarnImpcastToBool : InheritableAttr {
+ let Spellings = [GCC<"warn_impcast_to_bool">];
+ let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
aaron.ballman wrote:
> 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++.
Yeah, makes sense.
Comment at: include/clang/Basic/Attr.td:1140
@@ +1139,3 @@
+ let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
+ "ExpectedFunctionOrMethod">;
+ let Documentation = [WarnImpcastToBoolDocs];
aaron.ballman wrote:
> No need to specify the WarnDiag or ExpectedFunctionOrMethod arguments; they
> will be handled automatically.
I didn't know that, thanks.
Comment at: include/clang/Basic/AttrDocs.td:2055
@@ -2054,1 +2054,3 @@
}
+def WarnImpcastToBoolDocs : Documentation {
+ let Category = DocCatFunction;
zaks.anna wrote:
> 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.
OK, will do.
Comment at: include/clang/Basic/AttrDocs.td:2058
@@ +2057,3 @@
+ 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 he knows
what he is doing, therefore a warning should be triggered only for implicit
casts.
+
aaron.ballman wrote:
> You should manually wrap this to roughly the 80 col limit.
>
> Instead of "he", can you use "they" please?
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.
Comment at: include/clang/Basic/DiagnosticGroups.td:57
@@ -56,2 +56,3 @@
def DoublePromotion : DiagGroup<"double-promotion">;
+def UnsafeBoolConversion : DiagGroup<"unsafe-bool-conversion">;
def EnumTooLarge : DiagGroup<"enum-too-large">;
aaron.ballman wrote:
> I'm not certain this requires its own diagnostic group. This can probably be
> handled under `BoolConversion`
OK.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2259
@@ +2258,3 @@
+def warn_attribute_return_int_only : Warning<
+ "%0 attribute only applies to return values that are integers">,
+ InGroup;
aaron.ballman wrote:
> How about: ...only applies to integer return types?
Yeah, that sounds better.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2883
@@ +2882,3 @@
+ "implicit conversion turns non-bool into bool: %0 to %1">,
+ InGroup, DefaultIgnore;
+
aaron.ballman wrote:
> I don't think this should be a DefaultIgnore diagnostic -- if the user wrote
> the attribute, they should get the diagnostic when appropriate.
Makes sense.
Comment at: lib/Sema/SemaChecking.cpp:8262
@@ +8261,3 @@
+/// e.g. (x ? f : g)(y)
+if (isa(E)) {
+ FunctionDecl* fn = cast(E)->getDirectCallee();
aaron.ballman wrote:
> Should use `if (const auto *CE = dyn_cast(E)) {`
Done.
Comment at: lib/Sema/SemaChecking.cpp:8263-8264
@@ +8262,4 @@
+if (isa(E)) {
+ FunctionDecl* fn = cast(E)->getDirectCallee();
+
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
urusant updated this revision to Diff 71921.
urusant added a comment.
In https://reviews.llvm.org/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.
Repository:
rL LLVM
https://reviews.llvm.org/D24507
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDeclAttr.cpp
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
test/Analysis/ReturnNonBoolTest.c
test/Analysis/ReturnNonBoolTest.cpp
test/SemaCXX/ReturnNonBoolTestCompileTime.cpp
test/SemaObjC/ReturnNonBoolTestCompileTime.m
Index: test/SemaObjC/ReturnNonBoolTestCompileTime.m
===
--- /dev/null
+++ test/SemaObjC/ReturnNonBoolTestCompileTime.m
@@ -0,0 +1,6 @@
+// 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
Index: test/SemaCXX/ReturnNonBoolTestCompileTime.cpp
===
--- /dev/null
+++ test/SemaCXX/ReturnNonBoolTestCompileTime.cpp
@@ -0,0 +1,70 @@
+// 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
+retur
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
urusant updated this revision to Diff 71927.
Repository:
rL LLVM
https://reviews.llvm.org/D24507
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDeclAttr.cpp
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
test/Analysis/ReturnNonBoolTest.c
test/Analysis/ReturnNonBoolTest.cpp
test/SemaCXX/ReturnNonBoolTestCompileTime.cpp
test/SemaObjC/ReturnNonBoolTestCompileTime.m
Index: test/SemaObjC/ReturnNonBoolTestCompileTime.m
===
--- /dev/null
+++ test/SemaObjC/ReturnNonBoolTestCompileTime.m
@@ -0,0 +1,6 @@
+// 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
Index: test/SemaCXX/ReturnNonBoolTestCompileTime.cpp
===
--- /dev/null
+++ test/SemaCXX/ReturnNonBoolTestCompileTime.cpp
@@ -0,0 +1,70 @@
+// 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()) {}
+}
+
+
+
Index: test/Analysis/ReturnNonBoolTest.cpp
===
--- /dev/null
+++ test/Analysis/ReturnNonBoolTest.cpp
@@ -0,0 +1,88 @@
+// 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
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
urusant added inline comments. Comment at: lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp:50 @@ +49,3 @@ + if (!State->contains(SR)) return; + + ExplodedNode *N = C.generateErrorNode(C.getState()); I have just noticed that I didn't specify the style option when I ran it the first time. Now it should be fine. Comment at: test/ReturnNonBoolTest.c:7 @@ +6,3 @@ + +#ifdef __clang__ +#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool)) danielmarjamaki wrote: > sorry but why do you have a #ifdef __clang__ isn't it always defined? 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. Repository: rL LLVM https://reviews.llvm.org/D24507 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
urusant created this revision.
urusant added reviewers: zaks.anna, dcoughlin, jordan_rose, NoQ.
urusant added subscribers: cfe-commits, daviddrysdale.
urusant set the repository for this revision to rL LLVM.
Herald added subscribers: mgorny, beanz.
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
Repository:
rL LLVM
https://reviews.llvm.org/D24507
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/Sema/SemaChecking.cpp
lib/Sema/SemaDeclAttr.cpp
lib/StaticAnalyzer/Checkers/CMakeLists.txt
lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp
test/ReturnNonBoolTest.c
test/ReturnNonBoolTest.cpp
test/ReturnNonBoolTestCompileTime.cpp
Index: test/ReturnNonBoolTestCompileTime.cpp
===
--- /dev/null
+++ test/ReturnNonBoolTestCompileTime.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wunsafe-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 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 InvalidAttributeUsage() RETURNS_NON_BOOL; // expected-warning{{'warn_impcast_to_bool' attribute only applies to return values that are integers}}
\ No newline at end of file
Index: test/ReturnNonBoolTest.cpp
===
--- /dev/null
+++ test/ReturnNonBoolTest.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=alpha.core.ReturnNonBool -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()
