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

[Ted Kremenek]

+1

> On Jan 20, 2014, at 6:29 PM, Jordan Rose <jordan_rose at apple.com> wrote:
> 
> Let's do it in two patches, but yes, that sounds like a great test case and new feature to add. It might be nice to add it to TraversalDumper as well—it's just a debug checker that rarely comes in handy, but as Prashant noted it is a surprising inconsistency.
> 
> Thanks!
> Jordan
> 
> 
>> On Jan 20, 2014, at 6:22 , Zach Davis <zdavkeos at gmail.com> wrote:
>> 
>> 
>> 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
> 
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140121/807652e7/attachment.html>