<!DOCTYPE html>
<html>
<head>
<title></title>
</head>
<body><blockquote><div>This does look fairly straightforward, and fairly good! But I'm concerned<br></div>
<div>about the FunctionDecl parameter: the analyzer also allows<br></div>
<div>ObjCMethodDecls and BlockDecls to be analyzed as top-level entry points.<br></div>
<div>Perhaps a PointerUnion3 would be the best thing to do here, at least for<br></div>
<div>now? Or alternately, we allow people to specify which initial state<br></div>
<div>they're checking, either by having several callbacks or using a template<br></div>
<div>parameter. What do you think?<br></div>
</blockquote><div> </div>
<div>Indeed,
I haven't considered these. I personally really don't like the idea of
exposing a PointerUnion3. I think the best solution would be to use
templates, like check::PreStmt, except with Decl, of course.<br></div>
<div> </div>
<blockquote><div>Also, one nice thing about checkInitialState is that it can be non-const,<br></div>
<div>since it is guaranteed to run 0 or 1 times before the checker is used<br></div>
<div>elsewhere. :-)<br></div>
</blockquote><div> </div>
<div>True enough. I'll look into it.<br></div>
<div> </div>
<blockquote><div>I think a NULL initial state means a checker has decided not to analyze<br></div>
<div>this function. Weird and unlikely, but possible. (For example, it allows<br></div>
<div>us to move the -analyze-function option into a checker.)<br></div>
</blockquote><div> </div>
<div>Allowing a checker to make this decision on behalf of all the other checkers... I'm not absolutely certain I like that idea.<br></div>
<div>It
would indeed allow -analyze-function to become a checker, but I would
consider that a hack: conceptually, I wouldn't consider this the job of a
checker.<br></div>
<div> </div>
<div>Besides -analyze-function, what
would this enable? The only other thing I can think of is forcing the
analyzer off code paths that are somehow known to the checker to be
impossible. (I'm talking about allowing a checker to set a NULL state in
general, not just in checkInitialState). That might be practical, I can
imagine some uses, but it would also require considerably more effort
than this.<br></div>
<div> </div>
<div>I'll think some more about this, but for the time being I would disallow NULLs from checkInitialState.<br></div>
<div> </div>
<blockquote><div>Can you explain why you needed to move the getInitialState call<br></div>
<div>earlier? It seems dangerous to run checkers before we have a block<br></div>
<div>counter in place, since conjured symbols use the block count.<br></div>
</blockquote><div> </div>
<div>That
was probably a mistake. Initially I thought to call the
checkInitialState callback outside getInitialState, and so I did a bit
of organizing the code around. I did not think this could be the issue.
I'll fix it, thanks!<br></div>
<div> </div>
<blockquote><div>I'm not sold on this one yet. I mean, I get the idea, but it again seems<br></div>
<div>like something where we want a nicer interface than just "FunctionDecl"<br></div>
<div>(and at the very least need to handle ObjCMethodDecl as well). But we can<br></div>
<div>discuss this later.<br></div>
</blockquote><div id="sig19426269"><div class="signature"> </div>
<div class="signature">Also
taking ObjCMethodDecl and BlockDecl into consideration, I'd say lets
create an EntryPointInfo class, that would basically just wrap a
PointerUnion3 (as an internal implementation detail), but would provide
more straightforward accessor methods like getDecl, getFunctionDecl,
getBlockDecl, as well as a get<T> and is<T>. This could also
be generalized and put to use in checkInitialState, and even in
CallEvent - I believe all we have right now is a const Decl* getDecl()
there.<br></div>
<div class="signature">What do you think?<br></div>
<div class="signature"> </div>
</div>
<blockquote><div id="sig19426269">Finally, style notes (guessing this comes from bouncing between projects):<br></div>
</blockquote><div> </div>
<div class="signature">Yeah,
in general, I tried to use the style I saw in the surrounding code, but
I did not really pay attention to this. Will definitely fix it, thanks
for pointing it out.<br></div>
<div class="signature">At work, we're
using clang-format to take care of these issues for us. I guess I can
also set it up for when I'm working on Clang. You're using the LLVM
style, right?<br></div>
<div class="signature"> </div>
<blockquote><div class="signature">No, sorry. It's not that it's particularly destabilizing so much as it's
just well past the deadline for features and enhancements.<br></div>
</blockquote><div class="signature"> </div>
<div class="signature">Fair enough. I will not hurry then.<br></div>
<div class="signature"> </div>
<div>Thanks for the help!<br></div>
<div> </div>
<div id="sig19426269"><div class="signature">-- <br></div>
<div class="signature"> Gábor 'ShdNx' Kozár<br></div>
<div class="signature"> kozargabor@gmail.com<br></div>
<div class="signature"> </div>
</div>
<div> </div>
<div> </div>
<div>On Mon, Dec 16, 2013, at 22:10, Jordan Rose wrote:<br></div>
<blockquote type="cite"><div>No, sorry. It's not that it's particularly destabilizing so much as it's just well past the deadline for features and enhancements.<br></div>
<div> </div>
<div>Jordan<br></div>
<div> </div>
<div> </div>
<div><div>On Dec 16, 2013, at 13:07 , Gabor Kozar <<a href="mailto:kozargabor@gmail.com">kozargabor@gmail.com</a>> wrote:<br></div>
<div> </div>
<blockquote type="cite"><div><div>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.<br></div>
<div> <br></div>
<div>(Sorry for the short answer, had a long day.)<br></div>
<div> <br></div>
<div><div>-- <br></div>
<div> Gábor 'ShdNx' Kozár<br></div>
<div> <a href="mailto:kozargabor@gmail.com">kozargabor@gmail.com</a><br></div>
<div> <br></div>
</div>
<div> <br></div>
<div> <br></div>
<div>On Mon, Dec 16, 2013, at 18:50, Jordan Rose wrote:<br></div>
<blockquote type="cite"><div>This does look fairly straightforward, and fairly good! But I'm concerned<br></div>
<div>about the FunctionDecl parameter: the analyzer also allows<br></div>
<div>ObjCMethodDecls and BlockDecls to be analyzed as top-level entry points.<br></div>
<div>Perhaps a PointerUnion3 would be the best thing to do here, at least for<br></div>
<div>now? Or alternately, we allow people to specify which initial state<br></div>
<div>they're checking, either by having several callbacks or using a template<br></div>
<div>parameter. What do you think?<br></div>
<div> <br></div>
<div>Also, one nice thing about checkInitialState is that it can be non-const,<br></div>
<div>since it is guaranteed to run 0 or 1 times before the checker is used<br></div>
<div>elsewhere. :-)<br></div>
<div> <br></div>
<div> <br></div>
<blockquote><div>- 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?<br></div>
</blockquote><div> <br></div>
<div> <br></div>
<div>I think a NULL initial state means a checker has decided not to analyze<br></div>
<div>this function. Weird and unlikely, but possible. (For example, it allows<br></div>
<div>us to move the -analyze-function option into a checker.)<br></div>
<div> <br></div>
<blockquote><div>- 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.<br></div>
</blockquote><div> <br></div>
<div> <br></div>
<div>I agree. Can you explain why you needed to move the getInitialState call<br></div>
<div>earlier? It seems dangerous to run checkers before we have a block<br></div>
<div>counter in place, since conjured symbols use the block count.<br></div>
<div> <br></div>
<div>Do we have any checkers that can use this? Perhaps as proof-of-concept<br></div>
<div>you can move the check for main's argc and argv into a checker.<br></div>
<div> <br></div>
<div> <br></div>
<blockquote><div>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.<br></div>
</blockquote><div> <br></div>
<div> <br></div>
<div>I'm not sold on this one yet. I mean, I get the idea, but it again seems<br></div>
<div>like something where we want a nicer interface than just "FunctionDecl"<br></div>
<div>(and at the very least need to handle ObjCMethodDecl as well). But we can<br></div>
<div>discuss this later.<br></div>
<div> <br></div>
<div>I think you'd implement this by calling checkBeginFunction right after<br></div>
<div>checkInitialState and also from ExprEngine::inlineCall (right after<br></div>
<div>"State->enterStackFrame(...)")<br></div>
<div> <br></div>
<div> <br></div>
<div>Finally, style notes (guessing this comes from bouncing between<br></div>
<div>projects):<br></div>
<div> <br></div>
<blockquote><div>+ if(!State) return NULL; // exit early in case of unfeasible state<br></div>
</blockquote><div> <br></div>
<div> <br></div>
<div>Space after if, consequent statement on a separate line, comments also on<br></div>
<div>a separate line (and in the form of a complete sentence, with<br></div>
<div>capitalization and a period).<br></div>
<div> <br></div>
<div>Thank you for putting this together!<br></div>
<div>Jordan<br></div>
<div> <br></div>
<div> <br></div>
<div>On Dec 13, 2013, at 17:03 , Gabor Kozar <<a href="mailto:kozargabor@gmail.com">kozargabor@gmail.com</a>> wrote:<br></div>
<div> <br></div>
<blockquote><div>Hi Jordan,<br></div>
<div> <br></div>
<div>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.)<br></div>
<div> <br></div>
<div>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).<br></div>
<div> <br></div>
<div>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:<br></div>
<div>ProgramStateRef checkInitialState(const FunctionDecl*, ProgramStateRef) const<br></div>
<div> <br></div>
<div>There are a few things I'm not sure about:<br></div>
<div>- 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?<br></div>
<div>- 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.<br></div>
<div> <br></div>
<div>Please let me know what you think!<br></div>
<div> <br></div>
<div>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.<br></div>
<div> <br></div>
<div>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.)<br></div>
<div> <br></div>
<div>Thanks!<br></div>
<div> <br></div>
<div>-- <br></div>
<div>Gábor 'ShdNx' Kozár<br></div>
<div><a href="mailto:kozargabor@gmail.com">kozargabor@gmail.com</a><br></div>
<div> <br></div>
</blockquote><div> <br></div>
<p>Email had 1 attachment:<br></p><ul><li><code>patch-checkInitialState.diff</code><br></li><li> 6k (text/x-patch)<br></li></ul></blockquote></div>
</blockquote></div>
<div> </div>
<div> </div>
</blockquote></body>
</html>