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

[Jordan Rose]

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
>> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140120/371de6f8/attachment.html>