[PATCH] [Analyzer] Implementation of UnaryPlusChecker

[Adam Schnitzer]

Anna,

That makes sense, and sounds like a great project to get started on. I will
notify cfe-dev and send in incremental patches. Thanks for all the help!

Adam

On Fri, Apr 19, 2013 at 7:51 PM, Anna Zaks <ganna at apple.com> wrote:

>
> On Apr 19, 2013, at 4:09 PM, Adam Schnitzer <adamschn at umich.edu> wrote:
>
> Anna, working on the StreamChecker sounds like a great idea. In terms of
> work left to do on it, I'm assuming it's mostly cleanup and testing? It
> also seems like the pattern of ensuring a resource should be opened/closed
> once seems relevant beyond streams. Maybe it would make sense to refactor
> it to include a generic base checker which could be reused?
>
> Adam,
>
> There are 2 files: SimpleStreamChecker.cpp, which was used for the talk
> and StreamChecker.cpp, which has been written before the talk and does not
> follow some of the "good practices" on how a checker should be written. I
> would take SimpleStreamChecker as the base and extend it to handle more
> APIs, like the ones StreamChecker is checking. Another enhancement would be
> to make a bug report more expressive. For example, you could mention where
> a file was opened when reporting a resource leak, etc. Finally, you would
> run the checker over a bunch of real code and see what else needs to be
> improved. Please, send an email to the cfe-dev list stating that you are
> going to work on this checker as I've recommended this as a starter project
> to several people.
>
> It is true that there are other APIs that require similar checks. However,
> I would start with a specific checker and generalize it later on. You could
> try to develop it so that it could be easily generalized.
> (Note that the existing Malloc and SecKeyChainAPI checkers are similar to
> this one, but generalizing them all in one could be a separate project.)
>
> As you are working on this, please, send incremental patches to the
> cfe-commits list (and CC me and Jordan).
>
> Cheers,
> Anna.
>
> Also, I'm not sure if anyone has had a chance to take a look yet, but I've
> been having a little problem <http://llvm.org/bugs/show_bug.cgi?id=15774> with
> the BoolAssignmentChecker. I'm a bit stuck at the moment, but would be
> interested in extending that as well.
>
>
> Adam
>
> On Thu, Apr 18, 2013 at 2:56 PM, Anna Zaks <ganna at apple.com> wrote:
>
>> On Apr 17, 2013, at 8:33 PM, Adam Schnitzer <adamschn at umich.edu> wrote:
>>
>> Jordan, Thank you very much for the feedback, I have a few comments.
>>
>> *(1) Despite being on our list, this is probably better suited to a
>>> compiler warning.*
>>>
>>>
>> I agree, this warning might be better as a compiler warning. I chose to
>> implement this checker as a mainly to learn a bit about the analyzer. This
>> one was on the list and seemed like a good place to get started.
>>
>>
>> Sorry for having an under-specified checker in the list!
>>
>>
>>
>>> *(2) Despite being on our list, "unsigned" isn't actually the
>>> interesting thing to check.*
>>>
>>>
>> When I was reading the checker suggestion, I interpreted the purpose to
>> be a more conservative version of a check for unary '+', which, arguably,
>> is often dead code. For example, I have seen structures like this fairly
>> commonly:
>>
>> int array[] = {
>>   -3,
>>   -2,
>>   -1,
>>   +1
>> };
>>
>> Where the '+' is used for alignment, which we wouldn't want to warn
>> about. However, if that array was changed to unsigned, it might be a
>> legitimate warning. I thought the assumption was there's at least a decent
>> chance a unary '+' on unsigned is dead code. The place where I most
>> commonly it pop up was legitimate:
>>
>> char a = 'A';
>> cout << a << " ";  // print A
>> cout << +a;  // prints numerical value of 'A'
>>
>>
>> This is in line with what Jordan had mentioned. If we are writing a
>> checker/warning that warns on redundant operations (or operations that have
>> no effect), we would not warn in this case as there will be a promotion.
>>
>> It should be possible to write a check/warning that finds cases where the
>> unary plus has no effect by examining the AST. It could be a candidate for
>> a compiler warning, since the check could be fast and does not require
>> path-sensitive program exploration. Generally, compiler warnings are better
>> because they reach more users. If you are interested, you could reach out
>> to the clang community and see if there is an interest in such a warning.
>> You could also write it as a checker first, see what is the false positive
>> rate and rewrite this as a compiler warning is it seems useful.
>>
>> But I hadn't considered the checker was intended to target idempotent
>> or erroneous promotions. If that is the intent, then it seems challenging
>> to decide whether an expression is dead code, or to "force a load", as you
>> put it.
>>
>>
>> *(3) Macros and templates make this tricky.*
>>>
>>
>> I thought the that this might have been the reason why the checker was
>> listed as a potential checker, rather than a compiler warning. It does seem
>> like a fairly "noisy" warning. I have run it through some student
>> code. Unfortunately all warnings it produced were false positives, with the
>> exception of one situation similar to the one above.
>>
>>
>> If you are interested in writing the warning, you could look at your
>> results and see if the suggested changes would get rid of the false
>> positives.
>>
>>
>> At this point, I'd be fine with throwing this checker out, as its utility
>> does seem quite limited. If anyone has any ideas on how this checker can be
>> improved to be more useful, I would be interested to hear.
>>
>> On an unrelated note, do you have any recommendations for what might be a
>> approachable second checker?
>>
>>
>> I think the i++ checker that you've proposed originally would be good.
>>  You could also productize the StreamChecker, which would be path-sensitive
>> and not too difficult. Note sure if anyone else is working on that..
>>
>> Jordan, Anton, what do you think?
>>
>>
>> Adam
>>
>>
>>
>>
>>>
>>>
>>
>>> On Apr 12, 2013, at 23:53 , Adam Schnitzer <adamschn at umich.edu> wrote:
>>>
>>> This patch is an implementation of the
>>> proposed "different.UnaryPlusWithUnsigned", which I implemented
>>> as "alpha.deadcode.UnaryPlusWithUnsigned".
>>>
>>> It is implemented as a simple AST checker. However, it seems that unary
>>> '+' is often removed from the AST
>>> as dead code. So some of the basic test cases don't work.
>>>
>>> This is my first (real) patch, so any feedback or criticism is
>>> appreciated.
>>>
>>> Adam Schnitzer
>>> <UnaryPlusChecker.patch>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130419/02a72bea/attachment.html>