<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><blockquote type="cite"><div>On Jun 25, 2014, at 2:30 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div><meta http-equiv="Content-Type" content="text/html charset=windows-1252"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">I noticed one side-effect of this patch: we no longer optimize fully cases like indirect-goto.c test in CodeGen below.<div>Since foo and foo2 are no longer inlined constant folding of the result in main no longer happens.</div><div><br></div><div>Does anyone have an issue with this?</div></div></div></blockquote><div><br></div>I don’t. We’ve never attempted to inline functions containing indirect branches. I’m surprised that your testcase works now, but to really do it correctly, we would need to check that there were no remaining uses of the blockaddress that might escape the function. I’m not aware of any code that does that check, and without it, it’s not a safe optimization.</div><div><br><blockquote type="cite"><div><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br></div><div>Thanks</div><div>Gerolf<br><div><br></div><div><div style="margin: 0px; font-family: Menlo; color: rgb(83, 48, 225);">// RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s | grep "ret i32 2520"</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"><br></div><div style="margin: 0px; font-family: Menlo; color: rgb(52, 189, 38);">static<span style=""> </span>int<span style=""> foo(</span>unsigned<span style=""> i) {</span></div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #34bd26">void</span> *addrs[] = { &&L1, &&L2, &&L3, &&L4, &&L5 };</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #34bd26">int</span> res = <span style="color: #c33720">1</span>;</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"><br></div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">goto</span> *addrs[i];</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">L5</span>: res *= <span style="color: #c33720">11</span>;</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">L4</span>: res *= <span style="color: #c33720">7</span>;</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">L3</span>: res *= <span style="color: #c33720">5</span>;</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">L2</span>: res *= <span style="color: #c33720">3</span>;</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">L1</span>: res *= <span style="color: #c33720">2</span>;<span style="background-color: #af5f00"> </span></div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">return</span> res;</div><div style="margin: 0px; font-family: Menlo;">}</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"><br></div><div style="margin: 0px; font-family: Menlo; color: rgb(52, 189, 38);">static<span style=""> </span>int<span style=""> foo2(</span>unsigned<span style=""> i) {</span></div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #34bd26">static</span> <span style="color: #34bd26">const</span> <span style="color: #34bd26">void</span> *addrs[] = { &&L1, &&L2, &&L3, &&L4, &&L5 };</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #34bd26">int</span> res = <span style="color: #c33720">1</span>;</div><div style="margin: 0px; font-family: Menlo; background-color: rgb(175, 95, 0); min-height: 21px;"> <br class="webkit-block-placeholder"></div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">goto</span> *addrs[i];</div><div style="margin: 0px; font-family: Menlo;"><span style="color: #ce7924">L5</span>: res *= <span style="color: #c33720">11</span>;</div><div style="margin: 0px; font-family: Menlo;"><span style="color: #ce7924">L4</span>: res *= <span style="color: #c33720">7</span>;</div><div style="margin: 0px; font-family: Menlo;"><span style="color: #ce7924">L3</span>: res *= <span style="color: #c33720">5</span>;</div><div style="margin: 0px; font-family: Menlo;"><span style="color: #ce7924">L2</span>: res *= <span style="color: #c33720">3</span>;</div><div style="margin: 0px; font-family: Menlo;"><span style="color: #ce7924">L1</span>: res *= <span style="color: #c33720">2</span>;<span style="background-color: #af5f00"> </span></div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">return</span> res;</div><div style="margin: 0px; font-family: Menlo;">}</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"><br></div><div style="margin: 0px; font-family: Menlo;"><span style="color: #34bd26">int</span> main() {</div><div style="margin: 0px; font-family: Menlo;"> <span style="color: #ce7924">return</span> foo(<span style="color: #c33720">3</span>)+foo2(<span style="color: #c33720">4</span>);</div><div style="margin: 0px; font-family: Menlo;">}</div><div><br></div><div><br></div><div><font color="#4f7a28">IR before:</font></div><div><br></div><div><div style="margin: 0px; font-family: Menlo;">; Function Attrs: nounwind readnone</div><div style="margin: 0px; font-family: Menlo;">define i32 @main() #0 {</div><div style="margin: 0px; font-family: Menlo;">entry: </div><div style="margin: 0px; font-family: Menlo;"> ret i32 2520 </div><div style="margin: 0px; font-family: Menlo;">} </div></div><div><br></div><div><br></div><div><font color="#e63b7a">IR now:</font></div><div><br></div><div><div style="margin: 0px; font-family: Menlo;">; Function Attrs: nounwind readnone</div><div style="margin: 0px; font-family: Menlo;">define i32 @main() #0 {</div><div style="margin: 0px; font-family: Menlo;">entry: </div><div style="margin: 0px; font-family: Menlo;"> %call = tail call fastcc i32 @foo(i32 3)</div><div style="margin: 0px; font-family: Menlo;"> %call1 = tail call fastcc i32 @foo2(i32 4)</div><div style="margin: 0px; font-family: Menlo;"> %add = add nsw i32 %call1, %call</div><div style="margin: 0px; font-family: Menlo;"> ret i32 %add</div><div style="margin: 0px; font-family: Menlo;">}</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"> <br class="webkit-block-placeholder"></div><div style="margin: 0px; font-family: Menlo;">; Function Attrs: nounwind readnone</div><div style="margin: 0px; font-family: Menlo;">define internal fastcc i32 @foo(i32 %i) #0 {</div><div style="margin: 0px; font-family: Menlo;">entry: </div><div style="margin: 0px; font-family: Menlo;"> br label %L1 </div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"> <br class="webkit-block-placeholder"></div><div style="margin: 0px; font-family: Menlo;">L1: ; preds = %entry</div><div style="margin: 0px; font-family: Menlo;"> ret i32 210</div><div style="margin: 0px; font-family: Menlo;">}</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"><br></div><div style="margin: 0px; font-family: Menlo;">; Function Attrs: nounwind readnone</div><div style="margin: 0px; font-family: Menlo;">define internal fastcc i32 @foo2(i32 %i) #0 {</div><div style="margin: 0px; font-family: Menlo;">entry:</div><div style="margin: 0px; font-family: Menlo;"> br label %L1</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"><br></div><div style="margin: 0px; font-family: Menlo;">L1: ; preds = %entry</div><div style="margin: 0px; font-family: Menlo;"> ret i32 2310</div><div style="margin: 0px; font-family: Menlo;">} </div></div><div><br></div><div><br></div><div><div>On Jun 25, 2014, at 11:32 AM, Bob Wilson <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: Helvetica; font-size: 18px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite"><div><br class="Apple-interchange-newline">On Jun 25, 2014, at 10:47 AM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi<div><br></div><div>there have been reports on Ruby segfaults triggered by inlining functions with block label address taken like</div><div><br></div><div><br></div><div>//Ruby code snippet</div><div><div style="margin: 0px; font-family: Menlo;">vm_exec_core<span style="color: rgb(206, 121, 36);">()</span><span class="Apple-converted-space"> </span>{</div><div style="margin: 0px; font-family: Menlo;"> finish_insn_seq_0<span class="Apple-converted-space"> </span><span style="color: rgb(206, 121, 36);">=</span><span class="Apple-converted-space"> </span>&&INSN_LABEL_finish<span style="color: rgb(206, 121, 36);">;</span></div><div style="margin: 0px; font-family: Menlo;">INSN_LABEL_finish<span style="color: rgb(206, 121, 36);">:</span></div><div style="margin: 0px; font-family: Menlo;"> <span class="Apple-converted-space"> </span><span style="color: rgb(206, 121, 36);">;</span> </div><div style="margin: 0px; font-family: Menlo;">} </div></div><div style="margin: 0px; font-family: Menlo;"><br></div><div style="margin: 0px;"><span style="font-family: Menlo;">T</span>his kind of scenario can also happen when LLVM picks a subset of blocks for inlining,</div><div style="margin: 0px;">which is the case with the actual code in the Ruby environment.</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">Background:</div><div style="margin: 0px;">LLVM suppresses inlining for such functions when there is an indirect branch. The attached</div><div style="margin: 0px;">patch does so even when there is no indirect branch. Note that user code like above would not</div><div style="margin: 0px;">make much sense: using the global for jumping across function boundaries would be illegal. </div><div style="margin: 0px;"><br></div><div style="margin: 0px;">Why is there a segfault:</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">In the snipped above the block with the label is recognized as dead. So it is eliminated. Instead </div><div style="margin: 0px;">of a block address the cloner stores a constant (sic!) into the global resulting in the segfault (when</div><div style="margin: 0px;">the global is used in a goto).</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">Why did it work in the past then:</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">By luck. In older versions vm_exec_core was also inlined but the label address used</div><div style="margin: 0px;">was the block label address in vm_exec_core. So the global jump ended up in the original function </div><div style="margin: 0px;">rather than in the caller which accidentally happened to work.</div><div style="margin: 0px;"><br></div><div style="margin: 0px;"><br></div><div style="margin: 0px;">-Gerolf</div><div style="margin: 0px; font-family: Menlo;"><br></div><div style="margin: 0px; font-family: Menlo;"></div></div><span><0001-Suppress-inlining-when-the-block-address-is-taken.patch></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div style="margin: 0px; font-family: Menlo;"></div></div></div></blockquote></div><div style="font-family: Helvetica; font-size: 18px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br></div><span style="font-family: Helvetica; font-size: 18px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">The change is fine, but I still have more comments about comments….</span><div style="font-family: Helvetica; font-size: 18px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><div><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;">Inlining function with block label address taken can cause</div><div style="margin: 0px; font-family: Menlo;">many problem and requires a rich infrastructure to support</div><div style="margin: 0px; font-family: Menlo;">including escape analysis. At this point the safest approach</div><div style="margin: 0px; font-family: Menlo;">to address these problems is by blocking inlining from</div><div style="margin: 0px; font-family: Menlo;">happening.</div></blockquote><div><br></div>Can you please put more of the context into the commit message? You’ve got some good background in the email above, and it would be useful to have that in the commit.</div><div><br><blockquote type="cite"><div style="margin: 0px; font-family: Menlo; min-height: 14px;"><br></div><div style="margin: 0px; font-family: Menlo;"><a href="rdar://17245966">rdar://17245966</a></div><div style="margin: 0px; font-family: Menlo;">---</div><div style="margin: 0px; font-family: Menlo;"> lib/Analysis/IPA/InlineCost.cpp | 21 ++++++++++++++++++++-</div><div style="margin: 0px; font-family: Menlo;"> test/Transforms/Inline/blockaddress.ll | 5 +++--</div><div style="margin: 0px; font-family: Menlo;"> 2 files changed, 23 insertions(+), 3 deletions(-)</div><div style="margin: 0px; font-family: Menlo; min-height: 14px;"><br></div><div style="margin: 0px; font-family: Menlo;">diff --git a/lib/Analysis/IPA/InlineCost.cpp b/lib/Analysis/IPA/InlineCost.cpp</div><div style="margin: 0px; font-family: Menlo;">index 4536583..b079f46 100644</div><div style="margin: 0px; font-family: Menlo;">--- a/lib/Analysis/IPA/InlineCost.cpp</div><div style="margin: 0px; font-family: Menlo;">+++ b/lib/Analysis/IPA/InlineCost.cpp</div><div style="margin: 0px; font-family: Menlo;">@@ -1097,6 +1097,16 @@ bool CallAnalyzer::analyzeCall(CallSite CS) {</div><div style="margin: 0px; font-family: Menlo;"> BasicBlock *BB = BBWorklist[Idx];</div><div style="margin: 0px; font-family: Menlo;"> if (BB->empty())</div><div style="margin: 0px; font-family: Menlo;"> continue;</div><div style="margin: 0px; font-family: Menlo;">+ // Disallow inlining of functions when the address of a label is</div><div style="margin: 0px; font-family: Menlo;">+ // taken. Usually when a block address is taken there is also</div><div style="margin: 0px; font-family: Menlo;">+ // an indirect branch which effectively suppresses inlining.</div><div style="margin: 0px; font-family: Menlo;">+ // But when we partially inline and only take a subset of the blocks</div><div style="margin: 0px; font-family: Menlo;">+ // into account there may not be an indirect branch. As cautionary</div><div style="margin: 0px; font-family: Menlo;">+ // measure we suppress inlining when there is a block label address taken.</div><div style="margin: 0px; font-family: Menlo;">+ // See also the comments to visitIndirectBrInst() and isInlineViable().</div><div style="margin: 0px; font-family: Menlo;">+</div><div style="margin: 0px; font-family: Menlo;">+ if (BB->hasAddressTaken())</div><div style="margin: 0px; font-family: Menlo;">+ return false;</div></blockquote><div><br></div>Please remove the blank line between the comment and the code.</div><div><br></div><div>You should remove the comment sentence beginning with “As cautionary measure…”. It is not just a cautionary measure — it fixes a serious miscompile of Ruby.</div><div><br></div><div>I’m not sure it is necessary to cross-reference the comments in the other functions, but regardless, you should update the visitIndirectBrInst comment to remove this sentence: "And as a QOI issue, if someone is using a blockaddress without an indirectbr, and that reference somehow ends up in another function or global, we probably don't want to inline this function.” With your change here, that comment is no longer relevant.</div><div><br><blockquote type="cite"><div style="margin: 0px; font-family: Menlo; min-height: 14px;"><br></div><div style="margin: 0px; font-family: Menlo;"> // Analyze the cost of this block. If we blow through the threshold, this</div><div style="margin: 0px; font-family: Menlo;"> // returns false, and we can bail on out.</div></blockquote><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;">@@ -1279,7 +1289,16 @@ bool InlineCostAnalysis::isInlineViable(Function &F) {</div><div style="margin: 0px; font-family: Menlo;"> Attribute::ReturnsTwice);</div><div style="margin: 0px; font-family: Menlo;"> for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI) {</div><div style="margin: 0px; font-family: Menlo;"> // Disallow inlining of functions which contain an indirect branch.</div><div style="margin: 0px; font-family: Menlo;">- if (isa<IndirectBrInst>(BI->getTerminator()))</div><div style="margin: 0px; font-family: Menlo;">+ // Also disallow inlining of functions when the address of a label is</div><div style="margin: 0px; font-family: Menlo;">+ // taken. Usually when a block address is taken there is also</div><div style="margin: 0px; font-family: Menlo;">+ // an indirect branch which effectively suppresses inlining.</div><div style="margin: 0px; font-family: Menlo;">+ // But when there is no indirect branch the user wrote a function</div><div style="margin: 0px; font-family: Menlo;">+ // that takes the label address but does not jump to it within</div><div style="margin: 0px; font-family: Menlo;">+ // the function. This does not make much sense and could point</div><div style="margin: 0px; font-family: Menlo;">+ // to code that is not legal (eg. resulting in jump across function</div><div style="margin: 0px; font-family: Menlo;">+ // boundaries). The check here at least prevents inlining of such code.</div><div style="margin: 0px; font-family: Menlo;">+ // See also the comments to visitIndirectBrInst() and in analyzeCall().</div><div style="margin: 0px; font-family: Menlo;">+ if (isa<IndirectBrInst>(BI->getTerminator()) || BI->hasAddressTaken())</div><div style="margin: 0px; font-family: Menlo;"> return false;</div><div style="margin: 0px; font-family: Menlo; min-height: 14px;"><br></div><div style="margin: 0px; font-family: Menlo;"> for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;</div></blockquote></div><div><div style="margin: 0px; font-family: Menlo;"><br></div></div></div><div style="font-size: 18px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; margin: 0px; font-family: Menlo;"><span style="font-family: Helvetica;">This comment seems a little verbose to me. I would at least remove the cross-references to comments in other functions.</span></div></blockquote></div><br></div></div></div></div></blockquote></div><br></body></html>