[cfe-dev] : Clang Static Code Analyzer does not report 'Switch' Statement as a branch condition.

[Zach Davis]

I am finally getting a chance to look at this again.  For a
use-case/test-case I have added runCheckersForPreStmt to processSwitch and
modified the UndefBranchChecker.cpp code to also look at SwitchStmt's so I
get warning about this kind of thing:

int a[2];
switch (a[1]) { ...

>>>

undef_test.c:7:5: warning: Branch condition evaluates to a garbage value
    switch (a[1]) {
    ^~~~~~  ~~~~

Does this sound like a reasonable test case?
Should I add the UndefBranchChecker.cpp changes and corresponding test case
in misc-ps.c to the patch I submit?
Are there other use-cases I should be considering?

Zach


On Thu, Dec 12, 2013 at 11:26 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> That sounds right. Let me know if you have any more questions!
>
> Jordan
>
> On Dec 12, 2013, at 7:17 , Zach Davis <zdavkeos at gmail.com> wrote:
>
> Let me see if I understand the plan:
>
> 1. Add a runCheckersForPreStmt() call to ExprEngine::processBranch
> right after the call to runCheckersForBranchStmt(), effectively
> chaining the two together.  The loop can the evaluate the nodes
> returned from both calls?
>
> 2. Add a runCheckersForPreStmt() call to the top of
> ExprEngine::processSwitch.  The loop will then need to evaluate the
> returned nodes as well as explore and evaluate the switch body as it
> currently does?
>
> 3. Add a runCheckersForPreStmt() call to
> ExprEngine::processIndirectGoto and add a loop to evaluate any
> returned nodes.
>
> Zach
>
> > Hi, Zach. That's great to hear. The code lives in ExprEngine.cpp, and
> > you can see in functions like ExprEngine::VisitUnaryOperator
> > (ExprEngineC.cpp) that it basically just consists of a call to the
> > CheckerManager to run the pre-statement checks, then a loop over the
> > results to actually perform the evaluation. In
> > ExprEngine::processBranch the loop is already in place, so you just
> > need to add the second callback; ExprEngine::processSwitch also has a
> > loop but may need a bit more
> > restructuring. ExprEngine::processIndirectGoto is the last of these
> > and probably needs a completely new loop added.
>
>
> On Wed, Dec 11, 2013 at 12:08 PM, Jordan Rose <jordan_rose at apple.com>wrote:
>
>>
>> On Dec 6, 2013, at 10:07 , Zach Davis <zdavkeos at gmail.com> wrote:
>>
>> > > Right; what I meant was that we'd prefer to make this work rather
>> than adding switch statements to checkBranchCondition, but right now
>> neither one works. Sorry for the inconvenience.
>> > >
>> > > Jordan
>> >
>> > I had this same problem and ended up using an ast-matcher in a clang
>> tool instead to get the information I wanted.
>> >
>> > I would be interested in helping add this feature though.
>> >
>> >
>> > I found the bug report (18175), but could use some pointers to get
>> started on a patch.
>>
>> Hi, Zach. That's great to hear. The code lives in ExprEngine.cpp, and you
>> can see in functions like ExprEngine::VisitUnaryOperator (ExprEngineC.cpp)
>> that it basically just consists of a call to the CheckerManager to run the
>> pre-statement checks, then a loop over the results to actually perform the
>> evaluation. In ExprEngine::processBranch the loop is already in place, so
>> you just need to add the second callback; ExprEngine::processSwitch also
>> has a loop but may need a bit more restructuring.
>> ExprEngine::processIndirectGoto is the last of these and probably needs a
>> completely new loop added.
>>
>> Please feel free to ask more specific questions, and when you're ready
>> send the first iteration of your patch to cfe-commits.
>>
>> Thanks for picking this up!
>> Jordan
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140120/1c8507df/attachment.html>