<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><blockquote type="cite"><div>On Jun 30, 2014, at 3:21 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div><span style="font-family: Helvetica; font-size: 12px; 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;">On Mon, Jun 30, 2014 at 3:18 PM, Bob Wilson <</span><a href="mailto:bob.wilson@apple.com" style="font-family: Helvetica; font-size: 12px; 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;">bob.wilson@apple.com</a><span style="font-family: Helvetica; font-size: 12px; 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;">> 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; 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" style="font-family: Helvetica; font-size: 12px; 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>On Jun 27, 2014, at 4:26 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><br>I revised the comments and commit message of the Ruby fix.<br><br><br><0001-Suppress-inlining-when-the-block-address-is-taken.patch><br><br><br>Thanks. This looks close. Please go ahead and commit with fixes for the<br>following:<br><br>@@ -1097,6 +1094,14 @@ 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 taken.<br>+ // Usually when a block address is taken there is also an indirect<br>branch<br>+ // which effectively suppresses inlining. But when we partially inline<br>an\<br>d<br>+ // only take a subset of the blocks into account there may not be an<br>+ // indirect branch. Such code does not make much sense since a block<br>+ // address label is allowed to be a jump target only within a function.<br>+ // Worse, in one instance it caused a segmentation fault in a Ruby<br>build.<br>+ if (BB->hasAddressTaken()) return false;<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><br>You made some other changes since the last revision. Please put the return<br>on a separate line, like you had it before.<br><br>I’m still not happy with the comment. The last part does not provide enough<br>information to understand _why_ it caused the Ruby segfault, so it’s not<br>really adding any value. It’s also not clear what you mean here by<br>“partially inline”. Would you be OK to replace it with this?<br><br>“Disallow inlining a blockaddress. A blockaddress only has defined behavior<br>for an indirect branch in the same function, and we do not currently support<br>inlining indirect branches. But, the inliner may not see an indirect branch<br>that ends up being dead code at a particular call site. If the blockaddress<br>escapes the function, e.g., via a global variable, inlining may lead to an<br>invalid cross-function reference."<br><br></blockquote><br style="font-family: Helvetica; font-size: 12px; 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;"><span style="font-family: Helvetica; font-size: 12px; 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;">This comment is absolutely ideal. :)</span><br style="font-family: Helvetica; font-size: 12px; 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 style="font-family: Helvetica; font-size: 12px; 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;"><span style="font-family: Helvetica; font-size: 12px; 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;">Might want to add to the comment with why we can't support it right now?</span><br style="font-family: Helvetica; font-size: 12px; 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;"></div></blockquote><div><br></div>That seems like it would be more appropriate in lib/Analysis/README.txt, as a suggestion for future work.</div><div><br><blockquote type="cite"><div><br style="font-family: Helvetica; font-size: 12px; 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" style="font-family: Helvetica; font-size: 12px; 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;">@@ -1278,8 +1283,15 @@ bool InlineCostAnalysis::isInlineViable(Function &F)<br>{<br> F.getAttributes().hasAttribute(AttributeSet::FunctionIndex,<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>+ // Disallow inlining of functions which contain an indirect branch.<br>Also<br>+ // disallow inlining of functions when the address of a label is taken.<br>+ // Usually when a block address is taken there is also an indirect<br>branch<br>+ // which effectively suppresses inlining. But when there is no<br>indirect<br>+ // branch the user wrote a function that takes the label address but<br>does<br>+ // not jump to it within the function. This does not make much sense<br>and<br>+ // could possibly be harmful (e.g. when the address is assigned a<br>global<br>+ // variable which is used in a global (cross-funtion) goto).<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>I still find this comment to be less than helpful. The problem you describe<br>here has nothing to do with whether inlining is viable. You’re saying that<br>we should not inline functions with invalid code. Why does inlining make it<br>any worse? I’m not opposed to adding the check for hasAddressTaken(), but I<br>would prefer to just have the comment say: "Disallow inlining of functions<br>which contain indirect branches or blockaddresses."<br><br></blockquote><br style="font-family: Helvetica; font-size: 12px; 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;"><span style="font-family: Helvetica; font-size: 12px; 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;">Agree, might want to reference the longer comment above or just copy it?</span><br style="font-family: Helvetica; font-size: 12px; 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 style="font-family: Helvetica; font-size: 12px; 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;"><span style="font-family: Helvetica; font-size: 12px; 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;">-eric</span><br style="font-family: Helvetica; font-size: 12px; 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;"></div></blockquote><div><br></div>I’d really like to avoid copying it. The situation in the longer comment does not apply here. This is just a QOI issue to avoid crazy stuff in the face of invalid code.</div><div><br><blockquote type="cite"><div><br style="font-family: Helvetica; font-size: 12px; 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" style="font-family: Helvetica; font-size: 12px; 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>I continue to work on the indirect-goto.c case. The fix that would make it<br>happen is inverting the<br>order of invocation of global const prop and global optimizer:<br><br>Index: lib/Transforms/IPO/PassManagerBuilder.cpp<br>===================================================================<br>--- lib/Transforms/IPO/PassManagerBuilder.cpp (revision 211729)<br>+++ lib/Transforms/IPO/PassManagerBuilder.cpp (working copy)<br>@@ -156,9 +156,9 @@<br> if (!DisableUnitAtATime) {<br> addExtensionsToPM(EP_ModuleOptimizerEarly, MPM);<br><br>- MPM.add(createGlobalOptimizerPass()); // Optimize out global vars<br><br> MPM.add(createIPSCCPPass()); // IP SCCP<br>+ MPM.add(createGlobalOptimizerPass()); // Optimize out global vars<br> MPM.add(createDeadArgEliminationPass()); // Dead argument elimination<br><br> MPM.add(createInstructionCombiningPass());// Clean up after IPCP & DAE<br><br><br>When the global optimizer is invoked first, the indirect branch using the<br>global array has not<br>been eliminated. So the global can’t be eliminated. That is pretty straight<br>forward and the pass swap<br>looks reasonable but requires more scrutiny and careful testing. As<br>additional benefit the global<br>array would be removed from the IR.<br><br><br>-Gerolf<br><br>On Jun 25, 2014, at 6:01 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><br>Bob and I discussed this a bit more. Interprocedural const prop eliminates<br>the indirect branch. Then the labels. So when the<br>inliner kicks in it does not “see" any of the issues that could block it.<br><br>However, with the patch there must still be some information about block<br>label address taken kept around when a<br>global (static) array contains the label addresses. I verified that the<br>test case would work when foo2<br>also declared just a local array const void *addrs[] = { &&L1, &&L2, &&L3,<br>&&L4, &&L5 }.<br>Optimistically it seems like we might catch the global case also without<br>escape analysis,<br>but I have not looked into that code yet.<br><br>-Gerolf<br><br><br><br>On Jun 25, 2014, at 4:00 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:<br><br>Oh! It’s a front-end test. I didn’t even think to look there….<br><br>The test predates the introduction of indirect branches and has only had<br>superficial changes since then. We used to translate computed gotos into<br>switch statements. I’m still trying to figure out how this ever worked in<br>the past.<br><br>On Jun 25, 2014, at 3:50 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<br><br>Looking at the history it looks like Chris L. committed the test case in<br>tools/clang/test/CodeGen/indirect-goto.c to test<br>block address labels.<br><br>I’m fine with the train of reasoning:<br>there is an indirect branch in the source and we<br>never attempt to inline such functions. Furthermore for inlining functions<br>with block<br>label address taken we would need escape analysis. We don’t have it. So<br>let’s not inline.<br><br>It looks like everyone is signing up for<br>“Expect no optimization when block address labels are used”.<br><br>Cheers<br>Gerolf<br><br><br><br><br>On Jun 25, 2014, at 2:49 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br><br>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><br>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><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>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.</blockquote></div></blockquote></div><br></body></html>