'Re: [cfe-commits] r157762 - /cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [cfe-commits] r157762 - /cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
From:       David Blaikie <dblaikie () gmail ! com>
Date:       2012-05-31 18:35:17
Message-ID: CAENS6EtT7LsTUSKWa76p5An+NSt-K3yu2t4MXDXK-tD1JuvewA () mail ! gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]


On Thu, May 31, 2012 at 11:07 AM, Anna Zaks <ganna@apple.com> wrote:

> Author: zaks
> Date: Thu May 31 13:07:55 2012
> New Revision: 157762
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=157762&view=rev
> Log:
> [analyzer] Cleanup for r157721.
> We should lock the number of elements after the initial parsing is
> complete. Recursive AST visitors in AnalyzesConsumer and CallGarph can
> trigger lazy pch deserialization resulting in more calls to
> HandleTopLevelDecl and appending to the LocalTUDecls list. We should
> ignore those.
> 

Is it only a perf impact to visit them unnecessarily? or could this affect
(in)correctness as well? (in the latter case, a test case might be nice, if
it's practical to write one)

Thanks!
- David


> 
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
> 
> Modified: cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp?rev=157762&r1=157761&r2=157762&view=diff
>  
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp Thu May 31
> 13:07:55 2012
> @@ -230,7 +230,7 @@
> 
> /// \brief Build the call graph for all the top level decls of this TU
> and
> /// use it to define the order in which the functions should be visited.
> -  void HandleDeclsGallGraph();
> +  void HandleDeclsGallGraph(const unsigned LocalTUDeclsSize);
> 
> /// \brief Run analyzes(syntax or path sensitive) on the given function.
> /// \param Mode - determines if we are requesting syntax only or path
> @@ -311,18 +311,16 @@
> }
> }
> 
> -void AnalysisConsumer::HandleDeclsGallGraph() {
> +void AnalysisConsumer::HandleDeclsGallGraph(const unsigned
> LocalTUDeclsSize) {
> // Otherwise, use the Callgraph to derive the order.
> // Build the Call Graph.
> CallGraph CG;
> 
> // Add all the top level declarations to the graph.
> -  // Note: TraverseDecl may modify LocalTUDecls, but only by appending
> more
> -  // entries.  Thus we don't use an iterator, but rely on LocalTUDecls
> -  // random access.  By doing so, we automatically compensate for
> iterators
> -  // possibly being invalidated, although this is a bit slower.
> -  const unsigned n = LocalTUDecls.size();
> -  for (unsigned i = 0 ; i < n ; ++i) {
> +  // Note: CallGraph can trigger deserialization of more items from a pch
> +  // (though HandleInterestingDecl); triggering additions to LocalTUDecls.
> +  // We rely on random access to add the initially processed Decls to CG.
> +  for (unsigned i = 0 ; i < LocalTUDeclsSize ; ++i) {
> CG.addToCallGraph(LocalTUDecls[i]);
> }
> 
> @@ -414,13 +412,13 @@
> // entries.  Thus we don't use an iterator, but rely on LocalTUDecls
> // random access.  By doing so, we automatically compensate for
> iterators
> // possibly being invalidated, although this is a bit slower.
> -    const unsigned n = LocalTUDecls.size();
> -    for (unsigned i = 0 ; i < n ; ++i) {
> +    const unsigned LocalTUDeclsSize = LocalTUDecls.size();
> +    for (unsigned i = 0 ; i < LocalTUDeclsSize ; ++i) {
> TraverseDecl(LocalTUDecls[i]);
> }
> 
> if (Mgr->shouldInlineCall())
> -      HandleDeclsGallGraph();
> +      HandleDeclsGallGraph(LocalTUDeclsSize);
> 
> // After all decls handled, run checkers on the entire TranslationUnit.
> checkerMgr->runCheckersOnEndOfTranslationUnit(TU, *Mgr, BR);
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits@cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 


[Attachment #5 (text/html)]

<br><br><div class="gmail_quote">On Thu, May 31, 2012 at 11:07 AM, Anna Zaks <span \
dir="ltr">&lt;<a href="mailto:ganna@apple.com" \
target="_blank">ganna@apple.com</a>&gt;</span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
                solid;padding-left:1ex">
Author: zaks<br>
Date: Thu May 31 13:07:55 2012<br>
New Revision: 157762<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=157762&amp;view=rev" \
target="_blank">http://llvm.org/viewvc/llvm-project?rev=157762&amp;view=rev</a><br> \
Log:<br> [analyzer] Cleanup for r157721.<br>
We should lock the number of elements after the initial parsing is<br>
complete. Recursive AST visitors in AnalyzesConsumer and CallGarph can<br>
trigger lazy pch deserialization resulting in more calls to<br>
HandleTopLevelDecl and appending to the LocalTUDecls list. We should<br>
ignore those.<br></blockquote><div><br></div><div>Is it only a perf impact to visit \
them unnecessarily? or could this affect (in)correctness as well? (in the latter \
case, a test case might be nice, if it&#39;s practical to write one)</div> \
<div><br></div><div>Thanks!</div><div>- David</div><div> </div><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> <br>
Modified:<br>
    cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp<br>
<br>
Modified: cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp?rev=157762&amp;r1=157761&amp;r2=157762&amp;view=diff" \
target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Front \
end/AnalysisConsumer.cpp?rev=157762&amp;r1=157761&amp;r2=157762&amp;view=diff</a><br>

==============================================================================<br>

--- cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp (original)<br>
+++ cfe/trunk/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp Thu May 31 13:07:55 \
2012<br> @@ -230,7 +230,7 @@<br>
<br>
   /// \brief Build the call graph for all the top level decls of this TU and<br>
   /// use it to define the order in which the functions should be visited.<br>
-  void HandleDeclsGallGraph();<br>
+  void HandleDeclsGallGraph(const unsigned LocalTUDeclsSize);<br>
<br>
   /// \brief Run analyzes(syntax or path sensitive) on the given function.<br>
   /// \param Mode - determines if we are requesting syntax only or path<br>
@@ -311,18 +311,16 @@<br>
   }<br>
 }<br>
<br>
-void AnalysisConsumer::HandleDeclsGallGraph() {<br>
+void AnalysisConsumer::HandleDeclsGallGraph(const unsigned LocalTUDeclsSize) {<br>
   // Otherwise, use the Callgraph to derive the order.<br>
   // Build the Call Graph.<br>
   CallGraph CG;<br>
<br>
   // Add all the top level declarations to the graph.<br>
-  // Note: TraverseDecl may modify LocalTUDecls, but only by appending more<br>
-  // entries.  Thus we don&#39;t use an iterator, but rely on LocalTUDecls<br>
-  // random access.  By doing so, we automatically compensate for iterators<br>
-  // possibly being invalidated, although this is a bit slower.<br>
-  const unsigned n = LocalTUDecls.size();<br>
-  for (unsigned i = 0 ; i &lt; n ; ++i) {<br>
+  // Note: CallGraph can trigger deserialization of more items from a pch<br>
+  // (though HandleInterestingDecl); triggering additions to LocalTUDecls.<br>
+  // We rely on random access to add the initially processed Decls to CG.<br>
+  for (unsigned i = 0 ; i &lt; LocalTUDeclsSize ; ++i) {<br>
     CG.addToCallGraph(LocalTUDecls[i]);<br>
   }<br>
<br>
@@ -414,13 +412,13 @@<br>
     // entries.  Thus we don&#39;t use an iterator, but rely on LocalTUDecls<br>
     // random access.  By doing so, we automatically compensate for iterators<br>
     // possibly being invalidated, although this is a bit slower.<br>
-    const unsigned n = LocalTUDecls.size();<br>
-    for (unsigned i = 0 ; i &lt; n ; ++i) {<br>
+    const unsigned LocalTUDeclsSize = LocalTUDecls.size();<br>
+    for (unsigned i = 0 ; i &lt; LocalTUDeclsSize ; ++i) {<br>
       TraverseDecl(LocalTUDecls[i]);<br>
     }<br>
<br>
     if (Mgr-&gt;shouldInlineCall())<br>
-      HandleDeclsGallGraph();<br>
+      HandleDeclsGallGraph(LocalTUDeclsSize);<br>
<br>
     // After all decls handled, run checkers on the entire TranslationUnit.<br>
     checkerMgr-&gt;runCheckersOnEndOfTranslationUnit(TU, *Mgr, BR);<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
</blockquote></div><br>



_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread] 
