<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">I think this looks good, especially for an initial patch. We obviously will refine it over time.<div><br></div><div>One nit:</div><div><br></div><div> llvm::cl::ParseCommandLineOptions(argc, argv, "clang analyzer");</div><div><br></div><div>should be:</div><div><br></div><div> llvm::cl::ParseCommandLineOptions(argc, argv, "clang-wpa");<br><br><font class="Apple-style-span" face="Menlo" size="3"><span class="Apple-style-span" style="font-size: 11px; "><br></span></font></div><div><div><div>On Jul 14, 2009, at 10:03 PM, Zhongxing Xu wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>New files attached. To print decl names, a new mapping from caller's<br>nodes to their ASTContexts is added.<br><br>On Wed, Jul 15, 2009 at 8:33 AM, Ted Kremenek<<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>> wrote:<br><blockquote type="cite">On Jul 13, 2009, at 6:31 AM, Zhongxing Xu wrote:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This is an initial implementation of call graph building based on pch<br></blockquote><blockquote type="cite">reader and the new Program/Entity facility. It is very primitive but<br></blockquote><blockquote type="cite">functional.<br></blockquote><blockquote type="cite">Put the files under clang/tools/wpa (meaning 'whole program<br></blockquote><blockquote type="cite">analysis'). After 'make', a new command 'clang-analyze' will be<br></blockquote><blockquote type="cite">generated. It takes a list of .ast files and build call graph over<br></blockquote><blockquote type="cite">them.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Hi Zhongxing,<br></blockquote><blockquote type="cite">I would call the program 'clang-callgraph' so that people don't confuse it<br></blockquote><blockquote type="cite">with the "static analyzer". Comments inline.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">#include "CallGraph.h"<br></blockquote><blockquote type="cite">#include "clang/AST/ASTContext.h"<br></blockquote><blockquote type="cite">#include "clang/AST/StmtVisitor.h"<br></blockquote><blockquote type="cite">using namespace clang;<br></blockquote><blockquote type="cite">using namespace idx;<br></blockquote><blockquote type="cite">CallGraph *CallGraph::G = 0;<br></blockquote><blockquote type="cite">namespace {<br></blockquote><blockquote type="cite">class CGBuilder : public StmtVisitor<CGBuilder> {<br></blockquote><blockquote type="cite"> CallGraph &G;<br></blockquote><blockquote type="cite"> FunctionDecl *FD;<br></blockquote><blockquote type="cite">public:<br></blockquote><blockquote type="cite"> CGBuilder(CallGraph &g, FunctionDecl *fd)<br></blockquote><blockquote type="cite"> : G(g), FD(fd) {}<br></blockquote><blockquote type="cite"> void VisitCompoundStmt(CompoundStmt *S) {<br></blockquote><blockquote type="cite"> VisitChildren(S);<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"> void VisitCallExpr(CallExpr *CE);<br></blockquote><blockquote type="cite"> void VisitChildren(Stmt *S) {<br></blockquote><blockquote type="cite"> for (Stmt::child_iterator I=S->child_begin(), E=S->child_end(); I !=<br></blockquote><blockquote type="cite">E;++I)<br></blockquote><blockquote type="cite"> if (*I)<br></blockquote><blockquote type="cite"> static_cast<CGBuilder*>(this)->Visit(*I);<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite">};<br></blockquote><blockquote type="cite">}<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">You can make this a little shorter by using 'CFGRecStmtVisitor' instead of<br></blockquote><blockquote type="cite">StmtVisitor. It basically does the recursion for you.<br></blockquote><br>I did look at 'CFGRecStmtVisitor'. Call graph building does not need<br>CFG building. I think a plain StmtVisitor is enough.<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite">void CGBuilder::VisitCallExpr(CallExpr *CE) {<br></blockquote><blockquote type="cite"> const Entity *CallerEnt = Entity::get(FD, G.getProgram());<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Seems like you can make things a little faster by making 'CallerEnt' an<br></blockquote><blockquote type="cite">instance variable that is initialized to NULL, and then lazily initialize it<br></blockquote><blockquote type="cite">here if it is NULL. This will speed things up when there are no calls.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> CallGraphNode *Node = G.getOrInsertFunction(CallerEnt);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This too can probably be memoized, since it seems that a CGBuilder is built<br></blockquote><blockquote type="cite">on a per-function basis.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> Expr *Callee = CE->getCallee();<br></blockquote><blockquote type="cite"> if (CastExpr *CE = dyn_cast<CastExpr>(Callee))<br></blockquote><blockquote type="cite"> Callee = CE->getSubExpr();<br></blockquote><blockquote type="cite"> if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(Callee)) {<br></blockquote><blockquote type="cite"> Decl *D = DRE->getDecl();<br></blockquote><blockquote type="cite"> if (FunctionDecl *CalleeDecl = dyn_cast<FunctionDecl>(D)) {<br></blockquote><blockquote type="cite"> const Entity *Ent = Entity::get(CalleeDecl, G.getProgram());<br></blockquote><blockquote type="cite"> CallGraphNode *CalleeNode = G.getOrInsertFunction(Ent);<br></blockquote><blockquote type="cite"> Node->addCallee(ASTLocation(FD, CE), CalleeNode);<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite">}<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Seems reasonable.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">void CallGraph::addTU(const ASTUnit &AST) {<br></blockquote><blockquote type="cite"> if (!G)<br></blockquote><blockquote type="cite"> G = new CallGraph();<br></blockquote><blockquote type="cite"> const ASTContext &Ctx = AST.getASTContext();<br></blockquote><blockquote type="cite"> DeclContext *DC = Ctx.getTranslationUnitDecl();<br></blockquote><blockquote type="cite"> for (DeclContext::decl_iterator I = DC->decls_begin(), E =<br></blockquote><blockquote type="cite">DC->decls_end();<br></blockquote><blockquote type="cite"> I != E; ++I) {<br></blockquote><blockquote type="cite"> if (FunctionDecl *FD = dyn_cast<FunctionDecl>(*I)) {<br></blockquote><blockquote type="cite"> if (FD->isThisDeclarationADefinition()) {<br></blockquote><blockquote type="cite"> CGBuilder builder(*G, FD);<br></blockquote><blockquote type="cite"> builder.Visit(FD->getBody());<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite">}<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Seems reasonable. I'm not certain if this will handle code buried in C++<br></blockquote><blockquote type="cite">namespaces, but this implementation right now doesn't handle C++ anyway (so<br></blockquote><blockquote type="cite">it can be done later).<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">CallGraphNode *CallGraph::getOrInsertFunction(const Entity *F) {<br></blockquote><blockquote type="cite"> CallGraphNode *&Node = FunctionMap[F];<br></blockquote><blockquote type="cite"> if (Node)<br></blockquote><blockquote type="cite"> return Node;<br></blockquote><blockquote type="cite"> return Node = new CallGraphNode(F);<br></blockquote><blockquote type="cite">}<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Seems reasonable.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">#ifndef LLVM_CLANG_ANALYSIS_CALLGRAPH<br></blockquote><blockquote type="cite">#define LLVM_CLANG_ANALYSIS_CALLGRAPH<br></blockquote><blockquote type="cite">#include "clang/Index/ASTLocation.h"<br></blockquote><blockquote type="cite">#include "clang/Index/Entity.h"<br></blockquote><blockquote type="cite">#include "clang/Index/Program.h"<br></blockquote><blockquote type="cite">#include "clang/Frontend/ASTUnit.h"<br></blockquote><blockquote type="cite">#include <vector><br></blockquote><blockquote type="cite">#include <map><br></blockquote><blockquote type="cite">namespace clang {<br></blockquote><blockquote type="cite">class CallGraphNode {<br></blockquote><blockquote type="cite"> const idx::Entity *F;<br></blockquote><blockquote type="cite"> typedef std::pair<idx::ASTLocation, CallGraphNode*> CallRecord;<br></blockquote><blockquote type="cite"> std::vector<CallRecord> CalledFunctions;<br></blockquote><blockquote type="cite">public:<br></blockquote><blockquote type="cite"> CallGraphNode(const idx::Entity *f) : F(f) {}<br></blockquote><blockquote type="cite"> void addCallee(idx::ASTLocation L, CallGraphNode *Node) {<br></blockquote><blockquote type="cite"> CalledFunctions.push_back(std::make_pair(L, Node));<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite">};<br></blockquote><blockquote type="cite">class CallGraph {<br></blockquote><blockquote type="cite"> static CallGraph *G;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> idx::Program Prog;<br></blockquote><blockquote type="cite"> typedef std::map<const idx::Entity *, CallGraphNode *> FunctionMapTy;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Eventually, if you care about speed, you'll probably want to use a DenseMap.<br></blockquote><blockquote type="cite"> I don't see you using the sortedness property of std::map here.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> FunctionMapTy FunctionMap;<br></blockquote><blockquote type="cite">public:<br></blockquote><blockquote type="cite"> static CallGraph *get() { return G; }<br></blockquote><blockquote type="cite"> static void addTU(const ASTUnit &AST);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Is there a reason you are using static methods/instance variables? I know<br></blockquote><blockquote type="cite">this is a small program, but if you wanted to design this to be more<br></blockquote><blockquote type="cite">reusable I think you'll want to to avoid using them in this way.<br></blockquote><br>I originally wanted to use Singleton pattern. Now I changed to a plain<br>implementation.<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite"> idx::Program &getProgram() { return Prog; }<br></blockquote><blockquote type="cite"> CallGraphNode *getOrInsertFunction(const idx::Entity * F);<br></blockquote><blockquote type="cite">};<br></blockquote><blockquote type="cite">}<br></blockquote><blockquote type="cite">#endif<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">#include "CallGraph.h"<br></blockquote><blockquote type="cite">#include "clang/Basic/FileManager.h"<br></blockquote><blockquote type="cite">#include "clang/Index/TranslationUnit.h"<br></blockquote><blockquote type="cite">#include "llvm/Support/CommandLine.h"<br></blockquote><blockquote type="cite">#include "llvm/Support/raw_ostream.h"<br></blockquote><blockquote type="cite">using namespace clang;<br></blockquote><blockquote type="cite">using namespace idx;<br></blockquote><blockquote type="cite">static llvm::cl::list<std::string><br></blockquote><blockquote type="cite">InputFilenames(llvm::cl::Positional, llvm::cl::desc("<input AST files>"));<br></blockquote><blockquote type="cite">class TUnit : public TranslationUnit {<br></blockquote><blockquote type="cite">public:<br></blockquote><blockquote type="cite"> TUnit(ASTUnit *ast, const std::string &filename)<br></blockquote><blockquote type="cite"> : AST(ast), Filename(filename) {}<br></blockquote><blockquote type="cite"> ASTContext &getASTContext() { return AST->getASTContext(); }<br></blockquote><blockquote type="cite"> llvm::OwningPtr<ASTUnit> AST;<br></blockquote><blockquote type="cite"> std::string Filename;<br></blockquote><blockquote type="cite">};<br></blockquote><blockquote type="cite">int main(int argc, char **argv) {<br></blockquote><blockquote type="cite"> llvm::cl::ParseCommandLineOptions(argc, argv, "clang analyzer");<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">"clang analyzer" is the name people associate with the static analyzer. I'd<br></blockquote><blockquote type="cite">just use 'clang-callgraph', since that is what this tool does.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> FileManager FileMgr;<br></blockquote><blockquote type="cite"> std::vector<TUnit*> TUnits;<br></blockquote><blockquote type="cite"> if (InputFilenames.empty())<br></blockquote><blockquote type="cite"> return 0;<br></blockquote><blockquote type="cite"> for (unsigned i = 0, e = InputFilenames.size(); i != e; ++i) {<br></blockquote><blockquote type="cite"> const std::string &InFile = InputFilenames[i];<br></blockquote><blockquote type="cite"> std::string ErrMsg;<br></blockquote><blockquote type="cite"> llvm::OwningPtr<ASTUnit> AST;<br></blockquote><blockquote type="cite"> AST.reset(ASTUnit::LoadFromPCHFile(InFile, FileMgr, &ErrMsg));<br></blockquote><blockquote type="cite"> if (!AST) {<br></blockquote><blockquote type="cite"> llvm::errs() << "[" << InFile << "] error: " << ErrMsg << '\n';<br></blockquote><blockquote type="cite"> return 1;<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"> TUnit *TU = new TUnit(AST.take(), InFile);<br></blockquote><blockquote type="cite"> TUnits.push_back(TU);<br></blockquote><blockquote type="cite"> }<br></blockquote><blockquote type="cite"> for (unsigned i = 0, e = TUnits.size(); i != e; ++i)<br></blockquote><blockquote type="cite"> CallGraph::addTU(*(TUnits[i]->AST));<br></blockquote><blockquote type="cite"> CallGraph *CG = CallGraph::get();<br></blockquote><blockquote type="cite">}<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I think this has the potentially to be a nice little tool, but I think in<br></blockquote><blockquote type="cite">order for it to be useful it should produce some basic output. I think<br></blockquote><blockquote type="cite">printing out the callgraph would be nice. You can also defined some<br></blockquote><blockquote type="cite">GraphTraits for the CallGraph (similar to what we do in<br></blockquote><blockquote type="cite">clang/include/AST/CFG.h) to get automatic GraphViz visualization.<br></blockquote><blockquote type="cite">All in all I think this is cool, I can look forward to playing around with<br></blockquote><blockquote type="cite">it!<br></blockquote><span><clang-wpa.cpp></span><span><CallGraph.cpp></span><span><CallGraph.h></span></div></blockquote></div><br></div></body></html>