<div dir="ltr">On Wed, Jul 24, 2013 at 7:03 PM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Oops, no. Thanks for keeping me honest. Will add soon.</div></div></blockquote><div>
<br></div><div>Thanks! And if you have any ideas that can help Pavel fix this, it would be super appreciated :) I think that this will be pretty important for C++ for the analyzer going forwards (and thx for all your help so far, too!)</div>
<div><br></div><div>Cheers,</div><div>/Manuel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><br>
<div><div>On Jul 24, 2013, at 8:33 , Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:</div><br><blockquote type="cite"><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">
<div dir="ltr">Jordan, did you add a regression test for this?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 23, 2013 at 4:29 AM, Jordan Rose<span> </span><span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>I did get around to reverting this, Pavel. Apart from <a href="http://llvm.org/bugs/show_bug.cgi?id=16664" target="_blank">PR16664</a>, I realized that neither the binary operators nor their subexpressions are still live when the analyzer goes back through the chain of logical expressions to see which destructors to run. (You can see how this is working using the debug.DumpCFG checker.) Without the values previously computed for the expression, the analyzer won't actually be consistent about which branches to take.</div>
<div><br></div><div>I don't have an immediate solution in mind. It's sort of non-trivial to teach the liveness analysis that an expression is going to be revisited later, but the only alternative I can think of is crawling backwards through the evaluated blocks to find out which branch we took, which is a horrible idea because it could include inlined calls. If you come up with anything, please send it up for review!</div>
<span><font color="#888888"><div><br></div><div>Jordan</div></font></span><div><div><div><br></div><br><div><div>On Jul 22, 2013, at 19:15 , Jordan Rose <<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>> wrote:</div>
<br><blockquote type="cite">Author: jrose<br>Date: Mon Jul 22 21:15:11 2013<br>New Revision: 186925<br><br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project?rev=186925&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=186925&view=rev</a><br>
Log:<br>Revert "[analyzer] Add very limited support for temporary destructors"<br><br>The analyzer doesn't currently expect CFG blocks with terminators to be<br>empty, but this can happen when generating conditional destructors for<br>
a complex logical expression, such as (a && (b || Temp{})). Moreover,<br>the branch conditions for these expressions are not persisted in the<br>state. Even for handling noreturn destructors this needs more work.<br>
<br>This reverts r186498.<br><br>Modified:<br> cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp<br> cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp<br> cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp<br>
cfe/trunk/test/Analysis/analyzer-config.c<br> cfe/trunk/test/Analysis/analyzer-config.cpp<br> cfe/trunk/test/Analysis/dtor.cpp<br> cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp<br>
URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp?rev=186925&r1=186924&r2=186925&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp?rev=186925&r1=186924&r2=186925&view=diff</a><br>
==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp Mon Jul 22 21:15:11 2013<br>
@@ -119,7 +119,7 @@ bool AnalyzerOptions::getBooleanOption(O<br>bool AnalyzerOptions::includeTemporaryDtorsInCFG() {<br> return getBooleanOption(IncludeTemporaryDtorsInCFG,<br> "cfg-temporary-dtors",<br>
- /* Default = */ true);<br>+ /* Default = */ false);<br>}<br><br>bool AnalyzerOptions::mayInlineCXXStandardLibrary() {<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp<br>
URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=186925&r1=186924&r2=186925&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp?rev=186925&r1=186924&r2=186925&view=diff</a><br>
==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp Mon Jul 22 21:15:11 2013<br>
@@ -590,15 +590,7 @@ void ExprEngine::ProcessMemberDtor(const<br><br>void ExprEngine::ProcessTemporaryDtor(const CFGTemporaryDtor D,<br> ExplodedNode *Pred,<br>- ExplodedNodeSet &Dst) {<br>
-<br>- QualType varType = D.getBindTemporaryExpr()->getSubExpr()->getType();<br>-<br>- // FIXME: Inlining of temporary destructors is not supported yet anyway, so we<br>- // just put a NULL region for now. This will need to be changed later.<br>
- VisitCXXDestructor(varType, NULL, D.getBindTemporaryExpr(),<br>- /*IsBase=*/ false, Pred, Dst);<br>-}<br>+ ExplodedNodeSet &Dst) {}<br><br>void ExprEngine::Visit(const Stmt *S, ExplodedNode *Pred,<br>
ExplodedNodeSet &DstTop) {<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp?rev=186925&r1=186924&r2=186925&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp?rev=186925&r1=186924&r2=186925&view=diff</a><br>
==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp Mon Jul 22 21:15:11 2013<br>
@@ -807,12 +807,6 @@ bool ExprEngine::shouldInlineCall(const<br> AnalysisDeclContextManager &ADCMgr = AMgr.getAnalysisDeclContextManager();<br> AnalysisDeclContext *CalleeADC = ADCMgr.getContext(D);<br><br>- // Temporary object destructor processing is currently broken, so we never<br>
- // inline them.<br>- // FIME: Remove this once temp destructors are working.<br>- if ((*currBldrCtx->getBlock())[currStmtIdx].getAs<CFGTemporaryDtor>())<br>- return false;<br>-<br> // The auto-synthesized bodies are essential to inline as they are<br>
// usually small and commonly used. Note: we should do this check early on to<br> // ensure we always inline these calls.<br><br>Modified: cfe/trunk/test/Analysis/analyzer-config.c<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/analyzer-config.c?rev=186925&r1=186924&r2=186925&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/analyzer-config.c?rev=186925&r1=186924&r2=186925&view=diff</a><br>
==============================================================================<br>--- cfe/trunk/test/Analysis/analyzer-config.c (original)<br>+++ cfe/trunk/test/Analysis/analyzer-config.c Mon Jul 22 21:15:11 2013<br>@@ -6,7 +6,7 @@ void foo() { bar(); }<br>
<br>// CHECK: [config]<br>// CHECK-NEXT: cfg-conditional-static-initializers = true<br>-// CHECK-NEXT: cfg-temporary-dtors = true<br>+// CHECK-NEXT: cfg-temporary-dtors = false<br>// CHECK-NEXT: faux-bodies = true<br>// CHECK-NEXT: graph-trim-interval = 1000<br>
// CHECK-NEXT: ipa = dynamic-bifurcate<br><br>Modified: cfe/trunk/test/Analysis/analyzer-config.cpp<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/analyzer-config.cpp?rev=186925&r1=186924&r2=186925&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/analyzer-config.cpp?rev=186925&r1=186924&r2=186925&view=diff</a><br>
==============================================================================<br>--- cfe/trunk/test/Analysis/analyzer-config.cpp (original)<br>+++ cfe/trunk/test/Analysis/analyzer-config.cpp Mon Jul 22 21:15:11 2013<br>@@ -17,7 +17,7 @@ public:<br>
// CHECK-NEXT: c++-stdlib-inlining = true<br>// CHECK-NEXT: c++-template-inlining = true<br>// CHECK-NEXT: cfg-conditional-static-initializers = true<br>-// CHECK-NEXT: cfg-temporary-dtors = true<br>+// CHECK-NEXT: cfg-temporary-dtors = false<br>
// CHECK-NEXT: faux-bodies = true<br>// CHECK-NEXT: graph-trim-interval = 1000<br>// CHECK-NEXT: ipa = dynamic-bifurcate<br><br>Modified: cfe/trunk/test/Analysis/dtor.cpp<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dtor.cpp?rev=186925&r1=186924&r2=186925&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dtor.cpp?rev=186925&r1=186924&r2=186925&view=diff</a><br>
==============================================================================<br>--- cfe/trunk/test/Analysis/dtor.cpp (original)<br>+++ cfe/trunk/test/Analysis/dtor.cpp Mon Jul 22 21:15:11 2013<br>@@ -416,19 +416,4 @@ namespace NoReturn {<br>
f(&x);<br> *x = 47; // no warning<br> }<br>-<br>- void g2(int *x) {<br>- if (! x) NR();<br>- *x = 47; // no warning<br>- }<br>-<br>- void f3(int **x) {<br>- NR();<br>- }<br>-<br>- void g3() {<br>
- int *x;<br>- f3(&x);<br>- *x = 47; // no warning<br>- }<br>}<br><br>Modified: cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp?rev=186925&r1=186924&r2=186925&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp?rev=186925&r1=186924&r2=186925&view=diff</a><br>
==============================================================================<br>--- cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp (original)<br>+++ cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp Mon Jul 22 21:15:11 2013<br>
@@ -108,24 +108,6 @@ TestCtorInits::TestCtorInits()<br> : a(int(A()) + int(B()))<br> , b() {}<br><br>-class NoReturn {<br>-public:<br>- ~NoReturn() __attribute__((noreturn));<br>- void f();<br>-};<br>-<br>-void test_noreturn1() {<br>
- int a;<br>- NoReturn().f();<br>- int b;<br>-}<br>-<br>-void test_noreturn2() {<br>- int a;<br>- NoReturn(), 47;<br>- int b;<br>-}<br>-<br>// CHECK: [B1 (ENTRY)]<br>// CHECK: Succs (1): B0<br>// CHECK: [B0 (EXIT)]<br>
@@ -864,36 +846,3 @@ void test_noreturn2() {<br>// CHECK: [B0 (EXIT)]<br>// CHECK: Preds (1): B1<br><br>-// CHECK: [B3 (ENTRY)]<br>-// CHECK: Succs (1): B2<br>-// CHECK: [B1]<br>-// CHECK: 1: int b;<br>-// CHECK: Succs (1): B0<br>
-// CHECK: [B2]<br>-// CHECK: 1: int a;<br>-// CHECK: 2: NoReturn() (CXXConstructExpr, class NoReturn)<br>-// CHECK: 3: [B2.2] (BindTemporary)<br>-// CHECK: 4: [B2.3].f<br>-// CHECK: 5: [B2.4]()<br>-// CHECK: 6: ~NoReturn() (Temporary object destructor)<br>
-// CHECK: Preds (1): B3<br>-// CHECK: Succs (1): B0<br>-// CHECK: [B0 (EXIT)]<br>-// CHECK: Preds (2): B1 B2<br>-<br>-// CHECK: [B3 (ENTRY)]<br>-// CHECK: Succs (1): B2<br>-// CHECK: [B1]<br>-// CHECK: 1: int b;<br>
-// CHECK: Succs (1): B0<br>-// CHECK: [B2]<br>-// CHECK: 1: int a;<br>-// CHECK: 2: NoReturn() (CXXConstructExpr, class NoReturn)<br>-// CHECK: 3: [B2.2] (BindTemporary)<br>-// CHECK: 4: 47<br>-// CHECK: 5: ... , [B2.4]<br>
-// CHECK: 6: ~NoReturn() (Temporary object destructor)<br>-// CHECK: Preds (1): B3<br>-// CHECK: Succs (1): B0<br>-// CHECK: [B0 (EXIT)]<br>-// CHECK: Preds (2): B1 B2<br><br><br>_______________________________________________<br>
cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">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></div></div></div><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">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></blockquote>
</div></div></div></blockquote></div><br></div></div></div></blockquote></div><br></div></div>