Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
zaks.anna added inline comments. Comment at: test/Analysis/ReturnNonBoolTest.c:67 @@ +66,3 @@ + + if (rc < 0) +// error handling 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. 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
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
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
danielmarjamaki added a subscriber: danielmarjamaki.
Comment at: include/clang/Basic/AttrDocs.td:2055
@@ -2054,1 +2054,3 @@
}
+def WarnImpcastToBoolDocs : Documentation {
+ let Category = DocCatFunction;
I saw your email on cfe-dev. This sounds like a good idea to me.
Comment at: lib/StaticAnalyzer/Checkers/ReturnNonBoolChecker.cpp:49
@@ +48,3 @@
+ // needed.
+ if (!State->contains(SR)) return;
+
It seems you need to run clang-format on this file also.
Comment at: test/ReturnNonBoolTest.c:7
@@ +6,3 @@
+
+#ifdef __clang__
+#define RETURNS_NON_BOOL __attribute__((warn_impcast_to_bool))
sorry but why do you have a #ifdef __clang__ isn't it always defined?
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
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
+
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 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
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
zaks.anna added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:2055
@@ -2054,1 +2054,3 @@
}
+def WarnImpcastToBoolDocs : Documentation {
+ let Category = DocCatFunction;
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.
Comment at: test/ReturnNonBoolTest.c:74
@@ +73,3 @@
+ // no good way to get those from the program state.
+ if (restricted_wrap(2)) // expected-warning{{implicit cast to bool is
dangerous for this value}}
+return;
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 = C.getSValBuilder();
DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
return state->assume(svalBuilder.evalEQ(state, *val, zero))
"
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
Re: [PATCH] D24507: Add attribute for return values that shouldn't be cast to bool
aaron.ballman added a comment.
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.
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,
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++.
Comment at: include/clang/Basic/Attr.td:1140
@@ +1139,3 @@
+ let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
+ "ExpectedFunctionOrMethod">;
+ let Documentation = [WarnImpcastToBoolDocs];
No need to specify the WarnDiag or ExpectedFunctionOrMethod arguments; they
will be handled automatically.
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.
+
You should manually wrap this to roughly the 80 col limit.
Instead of "he", can you use "they" please?
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">;
I'm not certain this requires its own diagnostic group. This can probably be
handled under `BoolConversion`
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;
How about: ...only applies to integer return types?
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2883
@@ +2882,3 @@
+ "implicit conversion turns non-bool into bool: %0 to %1">,
+ InGroup, DefaultIgnore;
+
I don't think this should be a DefaultIgnore diagnostic -- if the user wrote
the attribute, they should get the diagnostic when appropriate.
Comment at: lib/Sema/SemaChecking.cpp:8262
@@ +8261,3 @@
+/// e.g. (x ? f : g)(y)
+if (isa(E)) {
+ FunctionDecl* fn = cast(E)->getDirectCallee();
Should use `if (const auto *CE = dyn_cast(E)) {`
Comment at: lib/Sema/SemaChecking.cpp:8263-8264
@@ +8262,4 @@
+if (isa(E)) {
+ FunctionDecl* fn = cast(E)->getDirectCallee();
+ if (!fn)
+return;
Then you can do `if (const auto *Fn = CE->getDirectCallee()) {`
Comment at: lib/Sema/SemaChecking.cpp:8269
@@ +8268,3 @@
+S.Diag(fn->getLocation(), diag::note_entity_declared_at)
+<< fn->getDeclName();
+return;
You can pass in `fn` directly, the diagnostics engine will properly get the
name out of it because it's derived from `NamedDecl`.
Comment at: lib/Sema/SemaDeclAttr.cpp:1316
@@ +1315,3 @@
+S.Diag(Attr.getLoc(), diag::warn_attribute_return_int_only)
+ <
