<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;">Thanks, Bob. <span style="font-family: Menlo;">Committed revision 212077. Please see below.</span><div><font face="Menlo"><br></font><div><div>On Jun 30, 2014, at 3:18 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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;"><br><div><blockquote type="cite"><div>On Jun 27, 2014, at 4:26 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 revised the comments and commit message of the Ruby fix. <br><div><br></div><div><br></div><div></div></div><span><0001-Suppress-inlining-when-the-block-address-is-taken.patch></span></div></blockquote><div><br></div>Thanks. This looks close. Please go ahead and commit with fixes for the following:</div><div><br></div><div><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;">@@ -1097,6 +1094,14 @@ 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 taken.</div><div style="margin: 0px; font-family: Menlo;">+ // Usually when a block address is taken there is also an indirect branch</div><div style="margin: 0px; font-family: Menlo;">+ // which effectively suppresses inlining. But when we partially inline an\</div><div style="margin: 0px; font-family: Menlo;">d</div><div style="margin: 0px; font-family: Menlo;">+ // only take a subset of the blocks into account there may not be an</div><div style="margin: 0px; font-family: Menlo;">+ // indirect branch. Such code does not make much sense since a block</div><div style="margin: 0px; font-family: Menlo;">+ // address label is allowed to be a jump target only within a function.</div><div style="margin: 0px; font-family: Menlo;">+ // Worse, in one instance it caused a segmentation fault in a Ruby build.</div><div style="margin: 0px; font-family: Menlo;">+ if (BB->hasAddressTaken()) return false;</div><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><div><div style="margin: 0px; font-family: Menlo;"><br></div></div><div style="margin: 0px;">You made some other changes since the last revision. Please put the return on a separate line, like you had it before.</div></div></div></blockquote>Yikes, when I forced the 80 character alignment it moved the return too…<br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div style="margin: 0px;"><br></div><div style="margin: 0px;">I’m still not happy with the comment. The last part does not provide enough information to understand _why_ it caused the Ruby segfault, so it’s not really adding any value. It’s also not clear what you mean here by “partially inline”. Would you be OK to replace it with this?</div><div style="margin: 0px;"><br></div><div style="margin: 0px;">“Disallow inlining a blockaddress. A blockaddress only has defined behavior for an indirect branch in the same function, and we do not currently support inlining indirect branches. But, the inliner may not see an indirect branch that ends up being dead code at a particular call site. If the blockaddress escapes the function, e.g., via a global variable, inlining may lead to an invalid cross-function reference.”</div></div></div></blockquote>Happy to steel that treasure of a comment for the benefit of the community :-) </div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div style="margin: 0px;"><br></div><div style="margin: 0px;"><blockquote type="cite"><div style="margin: 0px; font-family: Menlo;">@@ -1278,8 +1283,15 @@ bool InlineCostAnalysis::isInlineViable(Function &F) {</div><div style="margin: 0px; font-family: Menlo;"> F.getAttributes().hasAttribute(AttributeSet::FunctionIndex,</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;">+ // Disallow inlining of functions which contain an indirect branch. Also</div><div style="margin: 0px; font-family: Menlo;">+ // disallow inlining of functions when the address of a label is taken.</div><div style="margin: 0px; font-family: Menlo;">+ // Usually when a block address is taken there is also an indirect branch</div><div style="margin: 0px; font-family: Menlo;">+ // which effectively suppresses inlining. But when there is no indirect</div><div style="margin: 0px; font-family: Menlo;">+ // branch the user wrote a function that takes the label address but does</div><div style="margin: 0px; font-family: Menlo;">+ // not jump to it within the function. This does not make much sense and</div><div style="margin: 0px; font-family: Menlo;">+ // could possibly be harmful (e.g. when the address is assigned a global</div><div style="margin: 0px; font-family: Menlo;">+ // variable which is used in a global (cross-funtion) goto).</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 style="margin: 0px; font-family: Menlo;"><br></div></div><div style="margin: 0px;">I still find this comment to be less than helpful. The problem you describe here has nothing to do with whether inlining is viable. You’re saying that we should not inline functions with invalid code. Why does inlining make it any worse? I’m not opposed to adding the check for hasAddressTaken(), but I would prefer to just have the comment say: "Disallow inlining of functions which contain indirect branches or block addresses.”</div></div></div></div></blockquote>I like the brevity of your suggestion and went with it. </div><div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div style="margin: 0px;"><div style="margin: 0px; font-family: Menlo;"><span style="font-family: Helvetica;"><br></span></div></div><blockquote type="cite"><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;"><div></div><div><br></div><div>I continue to work on the indirect-goto.c case. The fix that would make it happen is inverting the </div><div>order of invocation of global const prop and global optimizer:</div><div><br></div><div><div style="margin: 0px; font-family: Menlo;">Index: lib/Transforms/IPO/PassManagerBuilder.cpp</div><div style="margin: 0px; font-family: Menlo;">===================================================================</div><div style="margin: 0px; font-family: Menlo;">--- lib/Transforms/IPO/PassManagerBuilder.cpp<span class="Apple-tab-span" style="white-space: pre;"> </span>(revision 211729)</div><div style="margin: 0px; font-family: Menlo;">+++ lib/Transforms/IPO/PassManagerBuilder.cpp<span class="Apple-tab-span" style="white-space: pre;"> </span>(working copy)</div><div style="margin: 0px; font-family: Menlo;">@@ -156,9 +156,9 @@</div><div style="margin: 0px; font-family: Menlo;"> if (!DisableUnitAtATime) {</div><div style="margin: 0px; font-family: Menlo;"> addExtensionsToPM(EP_ModuleOptimizerEarly, MPM);</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"> <br class="webkit-block-placeholder"></div><div style="margin: 0px; font-family: Menlo;">- MPM.add(createGlobalOptimizerPass()); // Optimize out global vars</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"> <br class="webkit-block-placeholder"></div><div style="margin: 0px; font-family: Menlo;"> MPM.add(createIPSCCPPass()); // IP SCCP</div><div style="margin: 0px; font-family: Menlo;">+ MPM.add(createGlobalOptimizerPass()); // Optimize out global vars</div><div style="margin: 0px; font-family: Menlo;"> MPM.add(createDeadArgEliminationPass()); // Dead argument elimination</div><div style="margin: 0px; font-family: Menlo; min-height: 21px;"> <br class="webkit-block-placeholder"></div><div style="margin: 0px; font-family: Menlo;"> MPM.add(createInstructionCombiningPass());// Clean up after IPCP & DAE</div><div style="margin: 0px; font-family: Menlo;"><br></div><div style="margin: 0px;"><br></div><div style="margin: 0px;">When the global optimizer is invoked first, the indirect branch using the global array has not</div><div style="margin: 0px;">been eliminated. So the global can’t be eliminated. That is pretty straight forward and the pass swap</div><div style="margin: 0px;">looks reasonable but requires more scrutiny and careful testing. As additional benefit the global</div></div><div style="margin: 0px;">array would be removed from the IR.</div><div style="margin: 0px;"><br></div><div style="margin: 0px;"><br></div><div>-Gerolf</div><div><br><div><div>On Jun 25, 2014, at 6:01 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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;">Bob and I discussed this a bit more. Interprocedural const prop eliminates the indirect branch. Then the labels. So when the<div><div>inliner kicks in it does not “see" any of the issues that could block it. </div><div><br></div><div>However, with the patch there must still be some information about block label address taken kept around when a </div><div>global (static) array contains the label addresses. I verified that the test case would work when foo2 </div><div>also declared just a local array const void *addrs[] = { &&L1, &&L2, &&L3, &&L4, &&L5 }.</div><div>Optimistically it seems like we might catch the global case also without escape analysis,</div><div>but I have not looked into that code yet.</div><div><br></div><div>-Gerolf</div><div><br></div><div><br></div><div><br></div><div><div><div>On Jun 25, 2014, at 4:00 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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;">Oh! It’s a front-end test. I didn’t even think to look there….<div><br></div><div>The test predates the introduction of indirect branches and has only had superficial changes since then. We used to translate computed gotos into switch statements. I’m still trying to figure out how this ever worked in the past.</div><div><br><div><blockquote type="cite"><div>On Jun 25, 2014, at 3:50 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;">Looking at the history it looks like Chris L. committed the test case in <span style="font-family: Menlo;">tools/clang/test/CodeGen/indirect-goto.c </span>to test <br><div>block address labels. </div><div><br></div><div>I’m fine with the train of reasoning: </div><div>there is an indirect branch in the source and we</div><div>never attempt to inline such functions. Furthermore for inlining functions with block</div><div>label address taken we would need escape analysis. We don’t have it. So let’s not inline.</div><div><br></div><div>It looks like everyone is signing up for </div><div>“Expect no optimization when block address labels are used”.</div><div><br></div><div>Cheers</div><div>Gerolf</div><div><br></div><div> </div><div><br></div><div><br></div><div><div><div>On Jun 25, 2014, at 2:49 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On Wed, Jun 25, 2014 at 2:46 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:<br><blockquote type="cite">On Jun 25, 2014, at 2:30 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><br>I noticed one side-effect of this patch: we no longer optimize fully cases<br>like indirect-goto.c test in CodeGen below.<br>Since foo and foo2 are no longer inlined constant folding of the result in<br>main no longer happens.<br><br>Does anyone have an issue with this?<br><br><br>I don’t. We’ve never attempted to inline functions containing indirect<br>branches. I’m surprised that your testcase works now, but to really do it<br>correctly, we would need to check that there were no remaining uses of the<br>blockaddress that might escape the function. I’m not aware of any code that<br>does that check, and without it, it’s not a safe optimization.<br><br><br></blockquote><br>Oh, wait, I see what the testcase is doing now. I'd thought it was<br>inlining into something that had blockaddr rather than the other way<br>around.<br><br>What does the commit for the testcase say about it?<br><br>Assuming it doesn't say anything enlightening then no, no issue here either.<br><br>-eric<br><br><blockquote type="cite">Thanks<br>Gerolf<br><br>// RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s |<br>grep "ret i32 2520"<br><br>static int foo(unsigned i) {<br> void *addrs[] = { &&L1, &&L2, &&L3, &&L4, &&L5 };<br> int res = 1;<br><br> goto *addrs[i];<br> L5: res *= 11;<br> L4: res *= 7;<br> L3: res *= 5;<br> L2: res *= 3;<br> L1: res *= 2;<br> return res;<br>}<br><br>static int foo2(unsigned i) {<br> static const void *addrs[] = { &&L1, &&L2, &&L3, &&L4, &&L5 };<br> int res = 1;<br><br> goto *addrs[i];<br>L5: res *= 11;<br>L4: res *= 7;<br>L3: res *= 5;<br>L2: res *= 3;<br>L1: res *= 2;<br> return res;<br>}<br><br>int main() {<br> return foo(3)+foo2(4);<br>}<br><br><br>IR before:<br><br>; Function Attrs: nounwind readnone<br>define i32 @main() #0 {<br>entry:<br> ret i32 2520<br>}<br><br><br>IR now:<br><br>; Function Attrs: nounwind readnone<br>define i32 @main() #0 {<br>entry:<br> %call = tail call fastcc i32 @foo(i32 3)<br> %call1 = tail call fastcc i32 @foo2(i32 4)<br> %add = add nsw i32 %call1, %call<br> ret i32 %add<br>}<br><br>; Function Attrs: nounwind readnone<br>define internal fastcc i32 @foo(i32 %i) #0 {<br>entry:<br> br label %L1<br><br>L1: ; preds = %entry<br> ret i32 210<br>}<br><br>; Function Attrs: nounwind readnone<br>define internal fastcc i32 @foo2(i32 %i) #0 {<br>entry:<br> br label %L1<br><br>L1: ; preds = %entry<br> ret i32 2310<br>}<br><br><br>On Jun 25, 2014, at 11:32 AM, Bob Wilson <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:<br><br><br>On Jun 25, 2014, at 10:47 AM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><br>Hi<br><br>there have been reports on Ruby segfaults triggered by inlining functions<br>with block label address taken like<br><br><br>//Ruby code snippet<br>vm_exec_core() {<br> finish_insn_seq_0 = &&INSN_LABEL_finish;<br>INSN_LABEL_finish:<br> ;<br>}<br><br>This kind of scenario can also happen when LLVM picks a subset of blocks for<br>inlining,<br>which is the case with the actual code in the Ruby environment.<br><br>Background:<br>LLVM suppresses inlining for such functions when there is an indirect<br>branch. The attached<br>patch does so even when there is no indirect branch. Note that user code<br>like above would not<br>make much sense: using the global for jumping across function boundaries<br>would be illegal.<br><br>Why is there a segfault:<br><br>In the snipped above the block with the label is recognized as dead. So it<br>is eliminated. Instead<br>of a block address the cloner stores a constant (sic!) into the global<br>resulting in the segfault (when<br>the global is used in a goto).<br><br>Why did it work in the past then:<br><br>By luck. In older versions vm_exec_core was also inlined but the label<br>address used<br>was the block label address in vm_exec_core. So the global jump ended up in<br>the original function<br>rather than in the caller which accidentally happened to work.<br><br><br>-Gerolf<br><br><0001-Suppress-inlining-when-the-block-address-is-taken.patch><br><br><br>The change is fine, but I still have more comments about comments….<br><br>Inlining function with block label address taken can cause<br>many problem and requires a rich infrastructure to support<br>including escape analysis. At this point the safest approach<br>to address these problems is by blocking inlining from<br>happening.<br><br><br>Can you please put more of the context into the commit message? You’ve got<br>some good background in the email above, and it would be useful to have that<br>in the commit.<br><br><br><a href="rdar://17245966">rdar://17245966</a><br>---<br> lib/Analysis/IPA/InlineCost.cpp | 21 ++++++++++++++++++++-<br> test/Transforms/Inline/blockaddress.ll | 5 +++--<br> 2 files changed, 23 insertions(+), 3 deletions(-)<br><br>diff --git a/lib/Analysis/IPA/InlineCost.cpp<br>b/lib/Analysis/IPA/InlineCost.cpp<br>index 4536583..b079f46 100644<br>--- a/lib/Analysis/IPA/InlineCost.cpp<br>+++ b/lib/Analysis/IPA/InlineCost.cpp<br>@@ -1097,6 +1097,16 @@ bool CallAnalyzer::analyzeCall(CallSite CS) {<br> BasicBlock *BB = BBWorklist[Idx];<br> if (BB->empty())<br> continue;<br>+ // Disallow inlining of functions when the address of a label is<br>+ // taken. Usually when a block address is taken there is also<br>+ // an indirect branch which effectively suppresses inlining.<br>+ // But when we partially inline and only take a subset of the blocks<br>+ // into account there may not be an indirect branch. As cautionary<br>+ // measure we suppress inlining when there is a block label address<br>taken.<br>+ // See also the comments to visitIndirectBrInst() and isInlineViable().<br>+<br>+ if (BB->hasAddressTaken())<br>+ return false;<br><br><br>Please remove the blank line between the comment and the code.<br><br>You should remove the comment sentence beginning with “As cautionary<br>measure…”. It is not just a cautionary measure — it fixes a serious<br>miscompile of Ruby.<br><br>I’m not sure it is necessary to cross-reference the comments in the other<br>functions, but regardless, you should update the visitIndirectBrInst comment<br>to remove this sentence: "And as a QOI issue, if someone is using a<br>blockaddress without an indirectbr, and that reference somehow ends up in<br>another function or global, we probably don't want to inline this function.”<br>With your change here, that comment is no longer relevant.<br><br><br> // Analyze the cost of this block. If we blow through the threshold,<br>this<br> // returns false, and we can bail on out.<br><br>@@ -1279,7 +1289,16 @@ bool InlineCostAnalysis::isInlineViable(Function &F)<br>{<br> Attribute::ReturnsTwice);<br> for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI) {<br> // Disallow inlining of functions which contain an indirect branch.<br>- if (isa<IndirectBrInst>(BI->getTerminator()))<br>+ // Also disallow inlining of functions when the address of a label is<br>+ // taken. Usually when a block address is taken there is also<br>+ // an indirect branch which effectively suppresses inlining.<br>+ // But when there is no indirect branch the user wrote a function<br>+ // that takes the label address but does not jump to it within<br>+ // the function. This does not make much sense and could point<br>+ // to code that is not legal (eg. resulting in jump across function<br>+ // boundaries). The check here at least prevents inlining of such code.<br>+ // See also the comments to visitIndirectBrInst() and in analyzeCall().<br>+ if (isa<IndirectBrInst>(BI->getTerminator()) || BI->hasAddressTaken())<br> return false;<br><br> for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;<br><br><br>This comment seems a little verbose to me. I would at least remove the<br>cross-references to comments in other functions.<br><br><br><br></blockquote></blockquote></div><br></div></div></div></blockquote></div><br></div></div></blockquote></div><br></div></div></div></blockquote></div><br></div></div></blockquote></div><br></div></blockquote></div><br></div></body></html>