<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Daniel,<div class=""><br class=""></div><div class="">This one prints "<span style="font-family: Menlo; font-size: 11px;" class="">Function: eee calls:" </span>twice. I think the check should be done inside getOrInsertNode (instead of VisitFunctionDecl). What do you think? If this looks good, I am going to go ahead and commit the patch. (I'll also replace CHECK with CHECK-NEXT.)</div><div class=""><br class=""></div><div class=""><div class=""><div style="margin: 0px; font-size: 14px; font-family: Menlo;" class=""><span style="color: rgb(79, 129, 135);" class="">CallGraphNode</span> *<span style="color: rgb(79, 129, 135);" class="">CallGraph</span>::getOrInsertNode(<span style="color: rgb(79, 129, 135);" class="">Decl</span> *F) {</div><div style="margin: 0px; font-size: 14px; font-family: Menlo;" class=""> <span style="color: rgb(187, 44, 162);" class="">if</span> (F && !<span style="color: rgb(61, 29, 129);" class="">isa</span><<span style="color: rgb(79, 129, 135);" class="">ObjCMethodDecl</span>>(F))</div><div style="margin: 0px; font-size: 14px; font-family: Menlo; color: rgb(49, 89, 93);" class=""><span style="color: rgb(0, 0, 0);" class=""> F = F-></span>getCanonicalDecl<span style="color: rgb(0, 0, 0);" class="">();</span></div></div></div><div class=""><span style="color: rgb(0, 0, 0);" class=""><br class=""></span></div><div class="">Anna.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 14px; font-family: Menlo;" class=""><br class=""></div></div><div class=""><div><blockquote type="cite" class=""><div class="">On Dec 4, 2014, at 2:20 AM, Daniel DeFreez <<a href="mailto:dcdefreez@ucdavis.edu" class="">dcdefreez@ucdavis.edu</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">OK, here's another go at it. This fixes the two test cases discussed, and doesn't break anything else that I can see. <div class=""><br class=""></div>Because callees do not always match the canonical declaration, I swap FunctionDecls for their canonical counterparts when they are visited. <br class=""><br class="">This differs from what we discussed by not using the canonical declaration for ObjCMethodDecls. This is because hasBody and getBody work differently for ObjCMethodDecls than FunctionDecls. Using the canonical declaration breaks the call graph. There isn't a problem with the call graph created for ObjCMethodDecls, so I have left them alone. Also for ObjCMethodDecls, isThisDeclarationADefinition is just { return hasBody(); }, so that includeInGraph check is redundant.<div class=""><br class=""></div><div class=""><div class="">Thoughts?<br class=""></div><div class=""><div class=""><br class=""></div><div class="">---<br class=""> include/clang/Analysis/CallGraph.h | 1 +<br class=""> lib/Analysis/CallGraph.cpp | 10 ++--------<br class=""> test/Analysis/debug-CallGraph.c | 14 +++++++++++++-<br class=""> 3 files changed, 16 insertions(+), 9 deletions(-)<br class=""><br class="">diff --git a/include/clang/Analysis/CallGraph.h b/include/clang/Analysis/CallGraph.h<br class="">index eda22a5..49aad7a 100644<br class="">--- a/include/clang/Analysis/CallGraph.h<br class="">+++ b/include/clang/Analysis/CallGraph.h<br class="">@@ -95,6 +95,7 @@ public:<br class=""> /// Part of recursive declaration visitation. We recursively visit all the<br class=""> /// declarations to collect the root functions.<br class=""> bool VisitFunctionDecl(FunctionDecl *FD) {<br class="">+ FD = FD->getCanonicalDecl();<br class=""> // We skip function template definitions, as their semantics is<br class=""> // only determined when they are instantiated.<br class=""> if (includeInGraph(FD)) {<br class="">diff --git a/lib/Analysis/CallGraph.cpp b/lib/Analysis/CallGraph.cpp<br class="">index f41a96d..5ea2dc3 100644<br class="">--- a/lib/Analysis/CallGraph.cpp<br class="">+++ b/lib/Analysis/CallGraph.cpp<br class="">@@ -110,14 +110,13 @@ CallGraph::~CallGraph() {<br class=""> <br class=""> bool CallGraph::includeInGraph(const Decl *D) {<br class=""> assert(D);<br class="">- if (!D->getBody())<br class="">+ if (!D->hasBody())<br class=""> return false;<br class=""> <br class=""> if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {<br class=""> // We skip function template definitions, as their semantics is<br class=""> // only determined when they are instantiated.<br class="">- if (!FD->isThisDeclarationADefinition() ||<br class="">- FD->isDependentContext())<br class="">+ if (FD->isDependentContext())<br class=""> return false;<br class=""> <br class=""> IdentifierInfo *II = FD->getIdentifier();<br class="">@@ -125,11 +124,6 @@ bool CallGraph::includeInGraph(const Decl *D) {<br class=""> return false;<br class=""> }<br class=""> <br class="">- if (const ObjCMethodDecl *ID = dyn_cast<ObjCMethodDecl>(D)) {<br class="">- if (!ID->isThisDeclarationADefinition())<br class="">- return false;<br class="">- }<br class="">-<br class=""> return true;<br class=""> }<br class=""> <br class="">diff --git a/test/Analysis/debug-CallGraph.c b/test/Analysis/debug-CallGraph.c<br class="">index 4523c78..ea7ea70 100644<br class="">--- a/test/Analysis/debug-CallGraph.c<br class="">+++ b/test/Analysis/debug-CallGraph.c<br class="">@@ -24,8 +24,20 @@ void bbb(int y) {<br class=""> }();<br class=""> }<br class=""> <br class="">+void ccc();<br class="">+void ddd() { ccc(); }<br class="">+void ccc() {}<br class="">+<br class="">+void eee();<br class="">+void eee() {}<br class="">+void fff() { eee(); }<br class="">+<br class=""> // CHECK:--- Call graph Dump ---<br class="">-// CHECK: Function: < root > calls: mmm foo aaa < > bbb<br class="">+// CHECK: Function: < root > calls: mmm foo aaa < > bbb ccc ddd eee fff<br class="">+// CHECK: {{Function: fff calls: eee $}}<br class="">+// CHECK: {{Function: eee calls: $}}<br class="">+// CHECK: {{Function: ddd calls: ccc $}}<br class="">+// CHECK: {{Function: ccc calls: $}}<br class=""> // CHECK: Function: bbb calls: < ><br class=""> // CHECK: Function: < > calls: foo<br class=""> // CHECK: Function: aaa calls: foo<br class="">-- <br class="">1.9.1<br class=""><br class=""><br class=""><br class=""></div></div></div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Sat, Nov 22, 2014 at 2:23 AM, Anna Zaks <span dir="ltr" class=""><<a href="mailto:ganna@apple.com" target="_blank" class="">ganna@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><div class=""><div class="h5"><blockquote type="cite" class=""><div class="">On Nov 21, 2014, at 5:21 PM, Daniel DeFreez <<a href="mailto:dcdefreez@ucdavis.edu" target="_blank" class="">dcdefreez@ucdavis.edu</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><div class="gmail_quote">On Sat, Nov 22, 2014 at 2:11 AM, Anna Zaks <span dir="ltr" class=""><<a href="mailto:ganna@apple.com" target="_blank" class="">ganna@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Nov 21, 2014, at 5:01 PM, Daniel DeFreez <<a href="mailto:dcdefreez@ucdavis.edu" target="_blank" class="">dcdefreez@ucdavis.edu</a>> wrote:</div><br class=""><div class=""><br class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">On Sat, Nov 22, 2014 at 12:48 AM, Anna Zaks<span class=""> </span></span><span dir="ltr" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><<a href="mailto:ganna@apple.com" target="_blank" class="">ganna@apple.com</a>></span><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class=""> </span><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">wrote:</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><blockquote class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Nov 21, 2014, at 3:35 PM, Daniel DeFreez <<a href="mailto:dcdefreez@ucdavis.edu" target="_blank" class="">dcdefreez@ucdavis.edu</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""> I believe with your patch, the node will get added, but the definition used will not have the body attached to it, so the analyzer will not process the body. </div></div></blockquote><div class=""><br class=""></div><div class="">Hmm, doesn't the analyzer itself use getBody() to access the body, thereby walking the list of redeclarations? It seems like it is still processing the body of the function, but maybe I don't know what to look for.</div></div></div></div></div></div></blockquote></span><div class="">This is a good point. We might not be gaining anything by replacing the decl with the one that has a body since getBody is guaranteed to search the redeclarations regardless.</div><div class=""><br class=""></div><div class="">How about just removing both calls to isThisDeclarationADefinition() (from includeInGraph) and replacing the call to getBody with hasBody (to avoid the unnecessary AST de-serialization of the body)? That should work and would simplify the function greatly. What do you think?</div></div></div></blockquote><div class=""><br class=""></div><div class="">I think replacing getBody() with hasBody() is a good idea regardless of the approach taken to fix the missing edges. And avoiding node replacement would make it cleaner.<br class=""></div><div class=""> <br class=""><br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""><div class="">I am not sure why some of the checks in your first patch were necessary (like this one).</div><span class=""><div class="">+ // Skip definitions with previous declarations</div><div class="">+ if (FD->getPreviousDecl())<br class="">+ return false;<br class="">+<br class=""></div></span><br class=""></div></div></blockquote><div class=""> </div></div></div><div class="gmail_extra">The reason I put that in was to avoid inserting redundant nodes. Just removing isThisDeclarationADefinition() yields:<br class=""><br class=""></div></div></div></blockquote><div class=""><br class=""></div></span>Oh, I see! getOrInsertNode() would not insert an existing declaration, but it does not check for re-declarations.</div><div class=""><br class=""></div><div class="">But would your approach work for this set?</div><div class="">void a();</div><div class=""><div class="">void a() { }</div>void b() { a(); } // Here, a call to "a()" might not be added since "a()" has a previous declaration.<br class=""></div></div></blockquote><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""> <br class="">Alas, yes, you're right. I can't believe I didn't try that. Not going to work.<br class=""><br class=""></div><blockquote class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="word-wrap:break-word" class=""><div class=""></div><div class="">Sounds like we would want to store canonical declarations, which brings us back to the suggestion of using "hasBody(Definition)" and only store the definitions that have bodies. Changing the signature of "includeInGraph" so that it mutates the Decl is fine; though, you might want to rename it as well.<br class=""><br class=""></div></div></blockquote><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""> </div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">Yep. The problem isn't with includeInGraph, though. It's with hasBody(Definition). Since it takes a reference to a pointer-to-const, using it get the canonical declaration means the canonical declaration itself must be const. So pretty much all of the Decl* in CallGraph would have to get changed to const.<br class=""><br class=""></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">I only know of two ways around it:<br class=""></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">1) Change the FunctionDecl class<br class=""></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class="">2) Walk redecls in some fashion similar to my second patch, which essentially just re-implemented hasBody without forcing the declaration to be const.<br class=""><br class="">Any other way?</div></div></blockquote></div><div class=""><br class=""></div></div></div>Looks like we can use getCanonicalDecl() instead (in both FunctionDecl and ObjCMethodDecl case).<br class=""><div class=""><div style="margin:0px;font-size:14px;font-family:Menlo" class=""><br class=""></div></div></div></blockquote><div class=""> </div><div class="">Ah! Excellent. How aptly named :)<br class=""><br class=""></div></div></div></div></div></blockquote><br class=""></div></div>:)</div><div class=""><br class=""></div><div class="">I'll be on vacation next week. </div><div class=""><br class=""></div><div class="">Seems like we have a solution here. I might be able to approve the patch, but won't be able to apply it. You should send the patch to cfe-dev and CC me.</div><div class=""><br class=""></div><div class="">Thank you!</div><div class="">Anna.</div><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class="">Many thanks.<br class=""></div></div><br class=""></div></div>
</div></blockquote></div><br class=""></div></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></body></html>