Hi clang,<br><br>Sorry for this late reply.<br><br>I reimplement UncheckedReturnChecker, here are some key points and some problems confusing me.<br><br><ol><li>Instead of the original syntactic approach, i track the symbolic values in BinaryOperator & UnaryOperator. It means every BinaryOperator & UnaryOperator will have a symbol to record the callexprs in subExpr. (So i create many Conjured Symbol to do this, is this all alright?)</li>
<li>I create a global DenseMap<const CallExpr*, CheckedReturnState*> to store whether a callexpr is checked or not.</li><li>This patch didn't consider the deal with the escaped symbol.</li><li>The bugreport is very simple now...<br>
</li><li>This checker seems much slower than other...</li>
<li>I didn't add any new action and programpoint yet...</li>
</ol>After all, there are many work should be done. I write this mail to ask whether this direction is right or not.<br><br>More comments inline.<br><br>I'll appreciate it if there are any advice about this patch.<br>
<br><br><div class="gmail_quote">2010/12/7 Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com" target="_blank">kremenek@apple.com</a>></span><br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div style="word-wrap: break-word;">Hi Lei,<div><br></div><div>I think this is a good first step. Comments inline.</div><div><br><div><div><div>On Dec 6, 2010, at 5:59 PM, ียภฺ wrote:</div><br><blockquote type="cite">
Hi clang,<br><br>I'm trying to find out how to implement this statistic checker. IMO, there could be several steps.<br><div style="margin-left: 40px;"><ol><li>Add a ConjuredSymbol to all the CallExpr in PostVisitCallExpr, set the state to Unchecked.</li>
</ol></div></blockquote></div><div>CallExprs are almost always guaranteed to evaluate to a symbolic value, so you don't need to create a new one. For example:</div><div><br></div><div><div>+void UncheckedReturnChecker::PostVisitCallExpr(CheckerContext &C, </div>
<div>+ const CallExpr *CE) {</div><div>+ unsigned Count = C.getNodeBuilder().getCurrentBlockCount();</div><div>+ SValBuilder &svalBuilder = C.getSValBuilder();</div><div>
+ SymbolRef Sym = svalBuilder.getConjuredSymbolVal(0, CE, Count).getAsSymbol();</div><div>+</div><div>+ const GRState *state = C.getState();</div><div>+</div><div>+ // Once we encounter a call expr, set the initial state to Unchecked.</div>
<div>+ state = </div><div>+ state->set<CheckedReturnState>(Sym, CheckedReturnState::getUnchecked(CE));</div><div>+</div><div>+ C.addTransition(state);</div><div>+ return;</div><div>+}</div><div><br></div><div>
Instead of conjuring a new symbol, try querying the SVal for the CallExpr. If it is a symbol, you can just add it to the GDM. For example:</div><div><br></div><div>const GRState *state = C.getState();</div><div>if (SymExpr *sym = state->getSVal(CE)->getAsSymbol()) {</div>
<div> state = state->set<CheckedReturnState>(sym, CheckReturnState::getUnchecked(CE));</div><div> C.addTransition(state);</div><div>}</div></div><div><br></div><div>The nice thing about this approach is that you associate your tracking state with the actual symbol returned by the CallExpr. That value is then tracked through assignments, etc.</div>
<div><br><blockquote type="cite"><div style="margin-left: 40px;"><ol start="2">
<li>In VisitBranchCondition, if a callexpr is checked, set the state to checked.</li></ol></div></blockquote></div><div>I think this logic looks alright, although it takes a highly syntactic approach instead of reasoning about symbolic values. One shortcoming of this approach is that it won't handle:</div>
<div><br></div><div> p = foo();</div><div> bool b = p == NULL;</div><div> if (b) { ... }</div><div><br></div><div>It is possible to handle this with a more sophisticated (albeit more complicated analysis) that reasons about symbolic values. Essentially, 'p == NULL' would evaluate to an SVal for which you could attach tracking state. Then, in VisitBranchCondition() you only need to reason about the SVal for the condition instead of its raw syntactic form.</div>
<div><br><blockquote type="cite"><div style="margin-left: 40px;"><ol start="3"><li>After all the decl was analyzed, do the first part(in a translationunit) of statistic count (and/or check) in AnalysisConsumer::HandleTranslationUnit.</li>
</ol></div></blockquote></div><div>Seems reasonable.</div><div><blockquote type="cite"><div style="margin-left: 40px;"><ol start="4">
<li>Implement a script to do the whole project's statistic check.</li></ol></div></blockquote></div>Seems reasonable.</div><div><div><br><blockquote type="cite">This patch try to do the first two steps. <br>
<br>I register the checker in RegisterExperimentalChecks, but a new action seems more appropriate. Statistic checkers are some different from the checkers in RegisterExperimentalChecks.<br></blockquote><div><br></div></div>
<div>Experimental checks is just a catch-all bucket for checks that are in development, but I agree with you that I think it makes sense to add a new flag for this case.</div><div><br><blockquote type="cite">
<br>In order to generate ExplodedNode but not a blockedge, i add a new "generateNode" function in GRBranchNodeBuilder. I think this function is not quite right. I use poststmt as the ProgramPoint, maybe we need a new ProgramPoint here?<br>
</blockquote><div><br></div></div><div>Yes, we'll likely need a new ProgramPoint. Since the condition is a Stmt*, it will have already been evaluated (with ExplodedNodes with a PostStmt program point). You might get unintended caching in the ExplodedGraph.</div>
<div><br><blockquote type="cite">
<br>Beacause this is a really experimental checker and not all the steps are finished, i can't give a testcase to show what i did. <br><br>More comments inline.<br></blockquote></div></div><br></div><div>One other nit is that the checker should implement 'evalDeadSymbols()' so that it clears out tracked symbols from the state. Otherwise we'll get artificial path explosion. Eventually we'll probably engineer the GDM so that it does this automatically for common cases.</div>
</div></blockquote></div><br><br clear="all"><br>-- <br>Best regards!<br><br>Lei Zhang<br>