[Clang Static Analyzer] Preliminary patch for checkInitialState

Mon Dec 16 13:07:28 PST 2013

Is there any chance of this making it into 3.4? If so, I can work (and
think) on it tomorrow, otherwise it can wait, obviously.

(Sorry for the short answer, had a long day.)

--
Gábor 'ShdNx' Kozár
kozargabor at gmail.com

On Mon, Dec 16, 2013, at 18:50, Jordan Rose wrote:

This does look fairly straightforward, and fairly good! But I'm
concerned

about the FunctionDecl parameter: the analyzer also allows

ObjCMethodDecls and BlockDecls to be analyzed as top-level entry
points.

Perhaps a PointerUnion3 would be the best thing to do here, at least
for

now? Or alternately, we allow people to specify which initial state

they're checking, either by having several callbacks or using a
template

parameter. What do you think?

Also, one nice thing about checkInitialState is that it can be
non-const,

since it is guaranteed to run 0 or 1 times before the checker is used

elsewhere. :-)

- Handling NULL ProgramStateRef-s. I do not think it is reasonable to
have NULL as the initial state. Probably assert when a
checkInitialState call returns NULL?

I think a NULL initial state means a checker has decided not to analyze

this function. Weird and unlikely, but possible. (For example, it
allows

us to move the -analyze-function option into a checker.)

- Whether to call the checkInitialState callback in
ExprEngine::getInitialState, or where getInitialState is used. I found
one such method: CoreEngine::ExecuteWorkList. Logically - knowing
nothing of the codebase -, I think it makes sense to do this in
getInitialState. It does not appear to belong to CoreEngine, especially
since I cannot find any references to getCheckerManager() in
CoreEngine.

I agree. Can you explain why you needed to move the getInitialState
call

earlier? It seems dangerous to run checkers before we have a block

counter in place, since conjured symbols use the block count.

Do we have any checkers that can use this? Perhaps as proof-of-concept

you can move the check for main's argc and argv into a checker.

Once checkInitialState is working, I also plan to do a void
checkBeginFunction(const FunctionDecl*, CheckerContext&) const, i.e. a
callback that is called whenever a function is entered, either through
a function call (this would be triggered right after checkPreCall) or
when it is the entry point of the analysis.

I'm not sold on this one yet. I mean, I get the idea, but it again
seems

like something where we want a nicer interface than just "FunctionDecl"

(and at the very least need to handle ObjCMethodDecl as well). But we
can

discuss this later.

I think you'd implement this by calling checkBeginFunction right after

checkInitialState and also from ExprEngine::inlineCall (right after

"State->enterStackFrame(...)")

Finally, style notes (guessing this comes from bouncing between

projects):

+    if(!State) return NULL; // exit early in case of unfeasible state

Space after if, consequent statement on a separate line, comments also
on

a separate line (and in the form of a complete sentence, with

capitalization and a period).

Thank you for putting this together!

Jordan

On Dec 13, 2013, at 17:03 , Gabor Kozar <[1]kozargabor at gmail.com>
wrote:

Hi Jordan,

I had an issue recently with the Static Analyzer that I wouldn't be
able to detect when a function was entered - either through a call or
serving as the entry point of the analysis. (I ended up using
checkPreStmt and figuring out if we've been magically transported
inside a function body and check the program state whether the
necessary info was already recorded by checkPreCall. Not something I'd
call nice by any means.)

So I wrote to cfg-dev and you mentioned that you had no such callbacks
currently implemented, although they have been needed before, and that
patches were welcome. Since this is a fairly trivial project, I was
comfortable with doing it in my very limited free time (especially
because I still intend to work on the Static Analyzer open projects at
some point when I'm able).

Find the diff file attached. Since you've pointed me to
ExprEngine::getInitialState, I just modified it to call any checkers
registered for check::InitialState at the end. So the signature for the
callback is:

ProgramStateRef checkInitialState(const FunctionDecl*, ProgramStateRef)
const

There are a few things I'm not sure about:

- Handling NULL ProgramStateRef-s. I do not think it is reasonable to
have NULL as the initial state. Probably assert when a
checkInitialState call returns NULL?

- Whether to call the checkInitialState callback in
ExprEngine::getInitialState, or where getInitialState is used. I found
one such method: CoreEngine::ExecuteWorkList. Logically - knowing
nothing of the codebase -, I think it makes sense to do this in
getInitialState. It does not appear to belong to CoreEngine, especially
since I cannot find any references to getCheckerManager() in
CoreEngine.

Please let me know what you think!

Once checkInitialState is working, I also plan to do a void
checkBeginFunction(const FunctionDecl*, CheckerContext&) const, i.e. a
callback that is called whenever a function is entered, either through
a function call (this would be triggered right after checkPreCall) or
when it is the entry point of the analysis.

I'm not sure about where can I notify the checkers of this though. When
is the point when we can say "okay so let's start analyzing this
function"? If you could point me to the method, that would help a lot.
(Ironically, we also have a code comprehension project running where I
work [at Ericsson], but I do not have access to server where we have
the Clang trunk parsed, since I'm doing this from home in my free
time.)

Thanks!

--

Gábor 'ShdNx' Kozár

[2]kozargabor at gmail.com

  Email had 1 attachment:
  * patch-checkInitialState.diff
  *   6k (text/x-patch)

References

1. mailto:kozargabor at gmail.com
2. mailto:kozargabor at gmail.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131216/85935026/attachment.html>