[cfe-dev] Several questions about UncheckedReturn statistic Checker

[章磊]

Hi clang,

Sorry for this late reply.

I reimplement UncheckedReturnChecker, here are some key points and some
problems confusing me.


   1. 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?)
   2. I create a global DenseMap<const CallExpr*, CheckedReturnState*> to
   store whether a callexpr is checked or not.
   3. This patch didn't consider the deal with the escaped symbol.
   4. The bugreport is very simple now...
   5. This checker seems much slower than other...
   6. I didn't add any new action and programpoint yet...

After all, there are many work should be done. I write this mail to ask
whether this direction is right or not.

More comments inline.

I'll appreciate it if there are any advice about this patch.


2010/12/7 Ted Kremenek <kremenek at apple.com>

> Hi Lei,
>
> I think this is a good first step.  Comments inline.
>
> On Dec 6, 2010, at 5:59 PM, 章磊 wrote:
>
> Hi clang,
>
> I'm trying to find out how to implement this statistic checker. IMO, there
> could be several steps.
>
>    1. Add a ConjuredSymbol to all the CallExpr in PostVisitCallExpr, set
>    the state to Unchecked.
>
> CallExprs are almost always guaranteed to evaluate to a symbolic value, so
> you don't need to create a new one.  For example:
>
> +void UncheckedReturnChecker::PostVisitCallExpr(CheckerContext &C,
> +                                               const CallExpr *CE) {
> +  unsigned Count = C.getNodeBuilder().getCurrentBlockCount();
> +  SValBuilder &svalBuilder = C.getSValBuilder();
> +  SymbolRef Sym = svalBuilder.getConjuredSymbolVal(0, CE,
> Count).getAsSymbol();
> +
> +  const GRState *state = C.getState();
> +
> +  // Once we encounter a call expr, set the initial state to Unchecked.
> +  state =
> +    state->set<CheckedReturnState>(Sym,
> CheckedReturnState::getUnchecked(CE));
> +
> +  C.addTransition(state);
> +  return;
> +}
>
> 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:
>
> const GRState *state = C.getState();
> if (SymExpr *sym = state->getSVal(CE)->getAsSymbol()) {
>   state = state->set<CheckedReturnState>(sym,
> CheckReturnState::getUnchecked(CE));
>   C.addTransition(state);
> }
>
> 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.
>
>
>    1. In VisitBranchCondition, if a callexpr is checked, set the state to
>    checked.
>
> 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:
>
>   p = foo();
>   bool b = p == NULL;
>   if (b) { ... }
>
> 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.
>
>
>    1. After all the decl was analyzed, do the first part(in a
>    translationunit) of statistic count (and/or check) in
>    AnalysisConsumer::HandleTranslationUnit.
>
> Seems reasonable.
>
>
>    1. Implement a script to do the whole project's statistic check.
>
> Seems reasonable.
>
> This patch try to do the first two steps.
>
> I register the checker in RegisterExperimentalChecks, but a new action
> seems more appropriate. Statistic checkers are some different from the
> checkers in RegisterExperimentalChecks.
>
>
> 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.
>
>
> 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?
>
>
> 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.
>
>
> 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.
>
> More comments inline.
>
>
> 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.
>



-- 
Best regards!

Lei Zhang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110310/915f71e9/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: UncheckedReturnChecker.patch
Type: text/x-patch
Size: 19167 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110310/915f71e9/attachment.bin>