<p dir="ltr">Yup. Thanks for the history there. I agree it shouldn't be inlined. </p>
<div class="gmail_quote">On Jun 25, 2014 3:17 PM, "Bob Wilson" <<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> On Jun 25, 2014, at 2:32 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:<br>
><br>
> Indirect goto is pretty important IIRC for threaded interpreters.<br>
> What's going on that we're not optimizing it anymore?<br>
><br>
> -eric<br>
<br>
When we added support for indirect branches, we intentionally ruled out inlining. It would be reasonable to add support for that optimization but I don’t think anyone has ever been sufficiently motivated to work on it.<br>
<br>
As far as I can tell, here is the sequence of what happened:<br>
<br>
1. We added indirect branches and taught the inliner to refuse to inline functions with indirect branches.<br>
<br>
2. Chandler enhanced the inliner to do constant folding and only look at portions of functions that are not dead code for particular constant arguments. This created a problem. If a function has an indirect branch that is dead code for a particular call site, the inliner won’t see it and may decide to inline anyway. The function may still have a blockaddress value that escapes, e.g., into a global variable. Ruby has exactly this situation.<br>
<br>
3. In the situation from (2), the blockaddress value in the inlined function is referring to a basic block that is otherwise dead code, and so that block gets removed. So then we have a blockaddress in the inlined function that is referring to a block in the original copy of the function. This is not valid. Until Gerolf’s change in r207713, this somehow happened to “work” and no one noticed the problem.<br>
<br>
4. Gerolf’s change r207713 landed, which caused the inliner to handle this in a more sane way, and exposed the problem in Ruby. That change has since been reverted, but the underlying issue is still there and that’s what Gerolf is trying to fix.<br>
<br>
So, this is not a case of a missing optimization, but rather a case where an optimization was supposed to be inhibited and we broke the check to turn it off.<br>
<br>
><br>
> On Wed, Jun 25, 2014 at 2:30 PM, Gerolf Hoflehner <<a href="mailto:ghoflehner@apple.com">ghoflehner@apple.com</a>> wrote:<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>
>> 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>
>><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>
>><br>
>><br>
>> ; Function Attrs: nounwind readnone<br>
>> define internal fastcc i32 @foo(i32 %i) #0 {<br>
>> entry:<br>
>> br label %L1<br>
>><br>
>><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>
>> rdar://17245966<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></div>