<div dir="ltr"><div><div><div><div>Let's suppose that we have 2 functions:<br></div>1st one:<br><br><div style="margin-left:40px">bool implCastToBoolWithBranching(int coin) {<br></div><div style="margin-left:40px"> char *str = 0;<br> if (coin) <br> str = "abc"; <br> return str; // warn<br></div><div style="margin-left:40px">}<br></div><div style="margin-left:40px"><br></div>2nd:<br><br><div style="margin-left:40px">bool explCastToBoolWithBranching(int coin) {<br></div><div style="margin-left:40px"> char *str = 0;<br> if (coin)<br> str = "abc";<br> bool b = str; // no-warn<br> return b;<br></div><div style="margin-left:40px">}</div><br></div>Current implementation isn't path-sensitive but it actually catches only implicit casts to boolean.<br></div>BTW, I'm going to file diff-review today, so we can actually discuss implementation details. Sorry for delay.<br><br></div>Thanks<br></div><div class="gmail_extra"><br><div class="gmail_quote">2017-12-04 1:30 GMT+03:00 Artem Dergachev <span dir="ltr"><<a href="mailto:noqnoqneo@gmail.com" target="_blank">noqnoqneo@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello,<br>
<br>
I asked to share the details because i suspected that you might be trying to implement an "all-paths" check, which is not a good idea.<br>
<br>
Consider:<br>
<br>
char *str = nullptr;<br>
if (coin)<span class=""><br>
str = "abc";<br>
bool b = str;<br>
<br></span>
On this example there exists a path (on which the coin is, say, 1) on which "abc" is converted to a boolean. However, the code is valid, and the checker should not flag it, because there exists another path (coin is 0) on which str is null, and the conversion is therefore non-trivial.<br>
<br>
The analyzer's path-sensitive engine is a great tool for finding singular paths on which certain events occur, however it is bad at finding situations when a property (invariant) holds on all paths. Even if you try to enumerate all paths traversed by the analyzer, the analyzer simply does not always explore all paths.<br>
<br>
There's no nice'n'easy tool in the analyzer to easily solve all-paths problems, you'd have to write the whole data flow analysis yourself if you want this to work (eg. DeadStores checker is made this way).<div><div class="h5"><br>
<br>
On 11/30/17 12:04 PM, Alexey Knyshev wrote:<br>
</div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
Good time of day!<br>
<br>
First of all I would like to show cases I've included in current test:<br>
<br>
const char *str = "abc";<br>
bool b = str; // no-warning<br>
...<br>
<br>
return str; // warning<br>
...<br>
return b; // no-warning<br>
<br>
bool implStrCast() {<br>
return "" // warn<br>
}<br>
<br>
<br>
const char *text() {<br>
return "some text";<br>
}<br>
<br>
bool implBoolCast() {<br>
return text(); // warning<br>
}<br>
<br>
bool explBoolCast() {<br>
bool r = text(); // no-warning<br>
return r; // no-warning<br>
}<br>
<br>
<br>
bool implFlootCast() {<br>
return 1.; // warning<br>
}<br>
<br>
<br>
bool explBoolCast2() {<br>
return text() == 0; // no-warning<br>
}<br>
<br>
<br>
bool boolProxy(int _, bool x, bool y);<br>
...<br>
boolProxy(1, getInteger(), (bool)1); // warn (only for second arg)<br>
<br>
<br>
I'd really like to know more about the approach you've taken with<br>
this checker. <br>
Current implementation checks 2 types of statements: CallExpr & ReturnStmt.<br>
First one actually aimed to check unexpected implicit casts of arguments passed to CallExpr (last example code). ReturnStmt for other presented cases. I could attach diff if it's appropriate here.<br>
<br>
Nowadays we try to make each test file correspond to a single checker.<br>
<br>
I'll consider separation of new checker into new one (including dedicated tests of course).<br>
<br>
But i don't think that there is a need for special code in the<br>
checker that detects testing environment and changes the behavior.<br>
<br></div></div>
I meant detecting & ignoring just special Calls (such as mentioned /clang_analyzer_eval/) only if we running in test env.<br>
<br>
Thanks, Alexey<br>
<br>
2017-11-30 19:34 GMT+03:00 Artem Dergachev <<a href="mailto:noqnoqneo@gmail.com" target="_blank">noqnoqneo@gmail.com</a> <mailto:<a href="mailto:noqnoqneo@gmail.com" target="_blank">noqnoqneo@gmail.com</a>>>:<div><div class="h5"><br>
<br>
(forgot to add cfe-dev on reply, re-replying, sry)<br>
<br>
Hello,<br>
<br>
I'd really like to know more about the approach you've taken with<br>
this checker. What kinds of conversions does it flag? Eg.:<br>
<br>
bool foo() {<br>
return ""; // warn.<br>
// or...<br>
char *str1 = "";<br>
return str1; // warn?<br>
// or...<br>
char *null_str = 0;<br>
return null_str; // nope.<br>
}<br>
<br>
It's not clear to me why would it warn on converting something to<br>
an int. In C, there doesn't seem to be an easy way to discriminate<br>
between int and boolean types. You might eventually want to<br>
hardcode a list of common library functions that treat their int<br>
arguments as boolean flags. And even if we have<br>
platform/library-specific typedefs such as ObjC BOOL, they are<br>
usually quite distinct from plain ints (eg. by typedef sugar).<br>
<br>
Then, ideally new checkers shouldn't be enabled on existing tests.<br>
Nowadays we try to make each test file correspond to a single<br>
checker. So you're running into a problem that arises because we<br>
didn't take this approach before, so we have some tests that have<br>
the whole alpha.core enabled (which is gross).<br>
<br>
If you're running into existing test file with a lot of eg.<br>
clang_analyzer_eval(BOOL) calls, and many of those suddenly get<br>
flagged, i think these are true positives. The checker works as<br>
intended, and we could just add expected-warnings on these lines.<br>
If there are too many such lines, i'd probably suggest to disable<br>
the checker for this file, i.e. adding<br>
-analyzer-disable-checker=alph<wbr>a.core.YourChecker to the run-line<br>
at the top of the file. Separate files for the checker should be<br>
enough.<br>
<br>
But i don't think that there is a need for special code in the<br>
checker that detects testing environment and changes the behavior.<br>
After all, that defeats the purpose of testing.<br>
<br>
On 11/29/17 12:20 PM, Alexey Knyshev via cfe-dev wrote:<br>
<br>
Hello everyone!<br>
<br>
I have implemented a prototype of<br>
"different.ConversionToBool(C, C++)".<br>
According to list of potential checkers<br>
(<a href="https://clang-analyzer.llvm.org/potential_checkers.html" rel="noreferrer" target="_blank">https://clang-analyzer.llvm.o<wbr>rg/potential_checkers.html</a><br>
<<a href="https://clang-analyzer.llvm.org/potential_checkers.html" rel="noreferrer" target="_blank">https://clang-analyzer.llvm.o<wbr>rg/potential_checkers.html</a>><br>
<<a href="https://clang-analyzer.llvm.org/potential_checkers.html" rel="noreferrer" target="_blank">https://clang-analyzer.llvm.o<wbr>rg/potential_checkers.html</a><br>
<<a href="https://clang-analyzer.llvm.org/potential_checkers.html" rel="noreferrer" target="_blank">https://clang-analyzer.llvm.o<wbr>rg/potential_checkers.html</a>>>)<br>
I've implemented it as the part of existing<br>
alpha.core.BoolAssignment checker. There is one problem I've<br>
faced while testing changes via llvm-lit. There are many<br>
FnChecks calls like /clang_analyzer_eval/ in<br>
Analysis/casts.[c/m] which are called with integer arguments<br>
instead of bool according declaration. Obviously it leads to<br>
false positives in implemented checker (implicit cast to<br>
boolean). Actually, the question is: What is the best way to<br>
avoid such false-positives? I came up with the following<br>
possible solutions:<br>
<br>
1. Is there way to distinguish FnCheck CallExpr from common<br>
CallExpr (any flag or whatever). If so, it's quite simple to<br>
ignore them in the checker.<br>
2. Is there way to determine if checker is running in testing<br>
env? And afterwards filter any calls to FnChecks<br>
like/clang_analyzer_eval /by name. As it was implemented in<br>
/CStringChecker:evalCall/.<br>
3. Add special FnCheck like<br>
/clang_analyser_eval_implicit_<wbr>cast(bool)/ which is supposed to<br>
replace /clang_analyzer_eval /in cast tests.<br>
<br>
Any suggestions?<br>
<br>
Thanks, Alexey K<br>
<br></div></div>
-- <a href="http://linkedin.com/profile" rel="noreferrer" target="_blank">linkedin.com/profile</a> <<a href="http://linkedin.com/profile" rel="noreferrer" target="_blank">http://linkedin.com/profile</a>><br>
<<a href="https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw" rel="noreferrer" target="_blank">https://www.linkedin.com/prof<wbr>ile/view?id=AAMAABn6oKQBDhBtei<wbr>QnWsYm-S9yxT7wQkfWhSw</a><span class=""><br>
<<a href="https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw" rel="noreferrer" target="_blank">https://www.linkedin.com/prof<wbr>ile/view?id=AAMAABn6oKQBDhBtei<wbr>QnWsYm-S9yxT7wQkfWhSw</a>>><br>
<br>
<a href="http://github.com/alexeyknyshev" rel="noreferrer" target="_blank">github.com/alexeyknyshev</a> <<a href="http://github.com/alexeyknyshev" rel="noreferrer" target="_blank">http://github.com/alexeyknysh<wbr>ev</a>><br></span>
<<a href="http://github.com/alexeyknyshev" rel="noreferrer" target="_blank">http://github.com/alexeyknysh<wbr>ev</a><br>
<<a href="http://github.com/alexeyknyshev" rel="noreferrer" target="_blank">http://github.com/alexeyknysh<wbr>ev</a>>><br>
<a href="http://bitbucket.org/alexeyknyshev" rel="noreferrer" target="_blank">bitbucket.org/alexeyknyshev</a><br>
<<a href="http://bitbucket.org/alexeyknyshev" rel="noreferrer" target="_blank">http://bitbucket.org/alexeykn<wbr>yshev</a>><br>
<<a href="https://bitbucket.org/alexeyknyshev/" rel="noreferrer" target="_blank">https://bitbucket.org/alexeyk<wbr>nyshev/</a><span class=""><br>
<<a href="https://bitbucket.org/alexeyknyshev/" rel="noreferrer" target="_blank">https://bitbucket.org/alexeyk<wbr>nyshev/</a>>><br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-dev mailing list<br></span>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a> <mailto:<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><wbr>><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-dev</a><br>
<<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin<wbr>/mailman/listinfo/cfe-dev</a>><span class=""><br>
<br>
<br>
<br>
<br>
<br>
-- <br>
<a href="http://linkedin.com/profile" rel="noreferrer" target="_blank">linkedin.com/profile</a> <<a href="https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw" rel="noreferrer" target="_blank">https://www.linkedin.com/prof<wbr>ile/view?id=AAMAABn6oKQBDhBtei<wbr>QnWsYm-S9yxT7wQkfWhSw</a>><br>
<br>
<a href="http://github.com/alexeyknyshev" rel="noreferrer" target="_blank">github.com/alexeyknyshev</a> <<a href="http://github.com/alexeyknyshev" rel="noreferrer" target="_blank">http://github.com/alexeyknysh<wbr>ev</a>><br>
<a href="http://bitbucket.org/alexeyknyshev" rel="noreferrer" target="_blank">bitbucket.org/alexeyknyshev</a> <<a href="https://bitbucket.org/alexeyknyshev/" rel="noreferrer" target="_blank">https://bitbucket.org/alexeyk<wbr>nyshev/</a>><br>
</span></blockquote>
<br>
</blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><a href="https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw" target="_blank">linkedin.com/profile</a><br><br><a href="http://github.com/alexeyknyshev" target="_blank">github.com/alexeyknyshev</a><span></span><a href="http:///" target="_blank"></a><span></span><br><a href="https://bitbucket.org/alexeyknyshev/" target="_blank">bitbucket.org/alexeyknyshev</a><br></div></div>
</div>