[cfe-dev] Clang Static Analyzer (different.ConversionToBool check)
Alexey Knyshev via cfe-dev
cfe-dev at lists.llvm.org
Sun Dec 3 21:50:28 PST 2017
Let's suppose that we have 2 functions:
1st one:
bool implCastToBoolWithBranching(int coin) {
char *str = 0;
if (coin)
str = "abc";
return str; // warn
}
2nd:
bool explCastToBoolWithBranching(int coin) {
char *str = 0;
if (coin)
str = "abc";
bool b = str; // no-warn
return b;
}
Current implementation isn't path-sensitive but it actually catches only
implicit casts to boolean.
BTW, I'm going to file diff-review today, so we can actually discuss
implementation details. Sorry for delay.
Thanks
2017-12-04 1:30 GMT+03:00 Artem Dergachev <noqnoqneo at gmail.com>:
> Hello,
>
> 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.
>
> Consider:
>
> char *str = nullptr;
> if (coin)
> str = "abc";
> bool b = str;
>
> 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.
>
> 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.
>
> 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).
>
>
> On 11/30/17 12:04 PM, Alexey Knyshev wrote:
>
>> Good time of day!
>>
>> First of all I would like to show cases I've included in current test:
>>
>> const char *str = "abc";
>> bool b = str; // no-warning
>> ...
>>
>> return str; // warning
>> ...
>> return b; // no-warning
>>
>> bool implStrCast() {
>> return "" // warn
>> }
>>
>>
>> const char *text() {
>> return "some text";
>> }
>>
>> bool implBoolCast() {
>> return text(); // warning
>> }
>>
>> bool explBoolCast() {
>> bool r = text(); // no-warning
>> return r; // no-warning
>> }
>>
>>
>> bool implFlootCast() {
>> return 1.; // warning
>> }
>>
>>
>> bool explBoolCast2() {
>> return text() == 0; // no-warning
>> }
>>
>>
>> bool boolProxy(int _, bool x, bool y);
>> ...
>> boolProxy(1, getInteger(), (bool)1); // warn (only for second arg)
>>
>>
>> I'd really like to know more about the approach you've taken with
>> this checker.
>> Current implementation checks 2 types of statements: CallExpr &
>> ReturnStmt.
>> 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.
>>
>> Nowadays we try to make each test file correspond to a single checker.
>>
>> I'll consider separation of new checker into new one (including dedicated
>> tests of course).
>>
>> But i don't think that there is a need for special code in the
>> checker that detects testing environment and changes the behavior.
>>
>> I meant detecting & ignoring just special Calls (such as mentioned
>> /clang_analyzer_eval/) only if we running in test env.
>>
>> Thanks, Alexey
>>
>> 2017-11-30 19:34 GMT+03:00 Artem Dergachev <noqnoqneo at gmail.com <mailto:
>> noqnoqneo at gmail.com>>:
>>
>>
>> (forgot to add cfe-dev on reply, re-replying, sry)
>>
>> Hello,
>>
>> I'd really like to know more about the approach you've taken with
>> this checker. What kinds of conversions does it flag? Eg.:
>>
>> bool foo() {
>> return ""; // warn.
>> // or...
>> char *str1 = "";
>> return str1; // warn?
>> // or...
>> char *null_str = 0;
>> return null_str; // nope.
>> }
>>
>> It's not clear to me why would it warn on converting something to
>> an int. In C, there doesn't seem to be an easy way to discriminate
>> between int and boolean types. You might eventually want to
>> hardcode a list of common library functions that treat their int
>> arguments as boolean flags. And even if we have
>> platform/library-specific typedefs such as ObjC BOOL, they are
>> usually quite distinct from plain ints (eg. by typedef sugar).
>>
>> Then, ideally new checkers shouldn't be enabled on existing tests.
>> Nowadays we try to make each test file correspond to a single
>> checker. So you're running into a problem that arises because we
>> didn't take this approach before, so we have some tests that have
>> the whole alpha.core enabled (which is gross).
>>
>> If you're running into existing test file with a lot of eg.
>> clang_analyzer_eval(BOOL) calls, and many of those suddenly get
>> flagged, i think these are true positives. The checker works as
>> intended, and we could just add expected-warnings on these lines.
>> If there are too many such lines, i'd probably suggest to disable
>> the checker for this file, i.e. adding
>> -analyzer-disable-checker=alpha.core.YourChecker to the run-line
>> at the top of the file. Separate files for the checker should be
>> enough.
>>
>> But i don't think that there is a need for special code in the
>> checker that detects testing environment and changes the behavior.
>> After all, that defeats the purpose of testing.
>>
>> On 11/29/17 12:20 PM, Alexey Knyshev via cfe-dev wrote:
>>
>> Hello everyone!
>>
>> I have implemented a prototype of
>> "different.ConversionToBool(C, C++)".
>> According to list of potential checkers
>> (https://clang-analyzer.llvm.org/potential_checkers.html
>> <https://clang-analyzer.llvm.org/potential_checkers.html>
>> <https://clang-analyzer.llvm.org/potential_checkers.html
>> <https://clang-analyzer.llvm.org/potential_checkers.html>>)
>> I've implemented it as the part of existing
>> alpha.core.BoolAssignment checker. There is one problem I've
>> faced while testing changes via llvm-lit. There are many
>> FnChecks calls like /clang_analyzer_eval/ in
>> Analysis/casts.[c/m] which are called with integer arguments
>> instead of bool according declaration. Obviously it leads to
>> false positives in implemented checker (implicit cast to
>> boolean). Actually, the question is: What is the best way to
>> avoid such false-positives? I came up with the following
>> possible solutions:
>>
>> 1. Is there way to distinguish FnCheck CallExpr from common
>> CallExpr (any flag or whatever). If so, it's quite simple to
>> ignore them in the checker.
>> 2. Is there way to determine if checker is running in testing
>> env? And afterwards filter any calls to FnChecks
>> like/clang_analyzer_eval /by name. As it was implemented in
>> /CStringChecker:evalCall/.
>> 3. Add special FnCheck like
>> /clang_analyser_eval_implicit_cast(bool)/ which is supposed to
>> replace /clang_analyzer_eval /in cast tests.
>>
>> Any suggestions?
>>
>> Thanks, Alexey K
>>
>> -- linkedin.com/profile <http://linkedin.com/profile>
>> <https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBtei
>> QnWsYm-S9yxT7wQkfWhSw
>> <https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBtei
>> QnWsYm-S9yxT7wQkfWhSw>>
>>
>> github.com/alexeyknyshev <http://github.com/alexeyknyshev>
>> <http://github.com/alexeyknyshev
>> <http://github.com/alexeyknyshev>>
>> bitbucket.org/alexeyknyshev
>> <http://bitbucket.org/alexeyknyshev>
>> <https://bitbucket.org/alexeyknyshev/
>> <https://bitbucket.org/alexeyknyshev/>>
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
>>
>>
>>
>>
>>
>> --
>> linkedin.com/profile <https://www.linkedin.com/prof
>> ile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw>
>>
>> github.com/alexeyknyshev <http://github.com/alexeyknyshev>
>> bitbucket.org/alexeyknyshev <https://bitbucket.org/alexeyknyshev/>
>>
>
>
--
linkedin.com/profile
<https://www.linkedin.com/profile/view?id=AAMAABn6oKQBDhBteiQnWsYm-S9yxT7wQkfWhSw>
github.com/alexeyknyshev
bitbucket.org/alexeyknyshev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20171204/670eaec6/attachment.html>
More information about the cfe-dev
mailing list