fix for Ruby build fails due to inlining functions with block label address taken

[Bob Wilson]

> On Jun 25, 2014, at 2:32 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
> Indirect goto is pretty important IIRC for threaded interpreters.
> What's going on that we're not optimizing it anymore?
> 
> -eric

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.

As far as I can tell, here is the sequence of what happened:

1. We added indirect branches and taught the inliner to refuse to inline functions with indirect branches.

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.

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.

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.

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.

> 
> On Wed, Jun 25, 2014 at 2:30 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>> I noticed one side-effect of this patch: we no longer optimize fully cases
>> like indirect-goto.c test in CodeGen below.
>> Since foo and foo2 are no longer inlined constant folding of the result in
>> main no longer happens.
>> 
>> Does anyone have an issue with this?
>> 
>> Thanks
>> Gerolf
>> 
>> // RUN: %clang_cc1 -triple i386-unknown-unknown -O3 -emit-llvm -o - %s |
>> grep "ret i32 2520"
>> 
>> static int foo(unsigned i) {
>>  void *addrs[] = { &&L1, &&L2, &&L3, &&L4, &&L5 };
>>  int res = 1;
>> 
>>  goto *addrs[i];
>> L5: res *= 11;
>> L4: res *= 7;
>> L3: res *= 5;
>> L2: res *= 3;
>> L1: res *= 2;
>>  return res;
>> }
>> 
>> static int foo2(unsigned i) {
>>  static const void *addrs[] = { &&L1, &&L2, &&L3, &&L4, &&L5 };
>>  int res = 1;
>> 
>> 
>> 
>>  goto *addrs[i];
>> L5: res *= 11;
>> L4: res *= 7;
>> L3: res *= 5;
>> L2: res *= 3;
>> L1: res *= 2;
>>  return res;
>> }
>> 
>> int main() {
>>  return foo(3)+foo2(4);
>> }
>> 
>> 
>> IR before:
>> 
>> ; Function Attrs: nounwind readnone
>> define i32 @main() #0 {
>> entry:
>>  ret i32 2520
>> }
>> 
>> 
>> IR now:
>> 
>> ; Function Attrs: nounwind readnone
>> define i32 @main() #0 {
>> entry:
>>  %call = tail call fastcc i32 @foo(i32 3)
>>  %call1 = tail call fastcc i32 @foo2(i32 4)
>>  %add = add nsw i32 %call1, %call
>>  ret i32 %add
>> }
>> 
>> 
>> 
>> ; Function Attrs: nounwind readnone
>> define internal fastcc i32 @foo(i32 %i) #0 {
>> entry:
>>  br label %L1
>> 
>> 
>> 
>> L1:                                               ; preds = %entry
>>  ret i32 210
>> }
>> 
>> ; Function Attrs: nounwind readnone
>> define internal fastcc i32 @foo2(i32 %i) #0 {
>> entry:
>>  br label %L1
>> 
>> L1:                                               ; preds = %entry
>>  ret i32 2310
>> }
>> 
>> 
>> On Jun 25, 2014, at 11:32 AM, Bob Wilson <bob.wilson at apple.com> wrote:
>> 
>> 
>> On Jun 25, 2014, at 10:47 AM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>> 
>> Hi
>> 
>> there have been reports on Ruby segfaults triggered by inlining functions
>> with block label address taken like
>> 
>> 
>> //Ruby code snippet
>> vm_exec_core() {
>>  finish_insn_seq_0 = &&INSN_LABEL_finish;
>> INSN_LABEL_finish:
>>  ;
>> }
>> 
>> This kind of scenario can also happen when LLVM picks a subset of blocks for
>> inlining,
>> which is the case with the actual code in the Ruby environment.
>> 
>> Background:
>> LLVM suppresses inlining for such functions when there is an indirect
>> branch. The attached
>> patch does so even when there is no indirect branch. Note that user code
>> like above would not
>> make much sense: using the global for jumping across function boundaries
>> would be illegal.
>> 
>> Why is there a segfault:
>> 
>> In the snipped above the block with the label is recognized as dead. So it
>> is eliminated. Instead
>> of a block address the cloner stores a constant (sic!) into the global
>> resulting in the segfault (when
>> the global is used in a goto).
>> 
>> Why did it work in the past then:
>> 
>> By luck. In older versions vm_exec_core was also inlined but the label
>> address used
>> was the block label address in vm_exec_core. So the global jump ended up in
>> the original function
>> rather than in the caller which accidentally happened to work.
>> 
>> 
>> -Gerolf
>> 
>> <0001-Suppress-inlining-when-the-block-address-is-taken.patch>
>> 
>> 
>> The change is fine, but I still have more comments about comments….
>> 
>> Inlining function with block label address taken can cause
>> many problem and requires a rich infrastructure to support
>> including escape analysis. At this point the safest approach
>> to address these problems is by blocking inlining from
>> happening.
>> 
>> 
>> 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.
>> 
>> 
>> rdar://17245966
>> ---
>> lib/Analysis/IPA/InlineCost.cpp        | 21 ++++++++++++++++++++-
>> test/Transforms/Inline/blockaddress.ll |  5 +++--
>> 2 files changed, 23 insertions(+), 3 deletions(-)
>> 
>> diff --git a/lib/Analysis/IPA/InlineCost.cpp
>> b/lib/Analysis/IPA/InlineCost.cpp
>> index 4536583..b079f46 100644
>> --- a/lib/Analysis/IPA/InlineCost.cpp
>> +++ b/lib/Analysis/IPA/InlineCost.cpp
>> @@ -1097,6 +1097,16 @@ bool CallAnalyzer::analyzeCall(CallSite CS) {
>>     BasicBlock *BB = BBWorklist[Idx];
>>     if (BB->empty())
>>       continue;
>> +    // Disallow inlining of functions when the address of a label is
>> +    // taken. Usually when a block address is taken there is also
>> +    // an indirect branch which effectively suppresses inlining.
>> +    // But when we partially inline and only take a subset of the blocks
>> +    // into account there may not be an indirect branch. As cautionary
>> +    // measure we suppress inlining when there is a block label address
>> taken.
>> +    // See also the comments to visitIndirectBrInst() and isInlineViable().
>> +
>> +    if (BB->hasAddressTaken())
>> +      return false;
>> 
>> 
>> Please remove the blank line between the comment and the code.
>> 
>> 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.
>> 
>> 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.
>> 
>> 
>>     // Analyze the cost of this block. If we blow through the threshold,
>> this
>>     // returns false, and we can bail on out.
>> 
>> @@ -1279,7 +1289,16 @@ bool InlineCostAnalysis::isInlineViable(Function &F)
>> {
>>                                    Attribute::ReturnsTwice);
>>   for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI) {
>>     // Disallow inlining of functions which contain an indirect branch.
>> -    if (isa<IndirectBrInst>(BI->getTerminator()))
>> +    // Also disallow inlining of functions when the address of a label is
>> +    // taken. Usually when a block address is taken there is also
>> +    // an indirect branch which effectively suppresses inlining.
>> +    // But when there is no indirect branch the user wrote a function
>> +    // that takes the label address but does not jump to it within
>> +    // the function. This does not make much sense and could point
>> +    // to code that is not legal (eg. resulting in jump across function
>> +    // boundaries). The check here at least prevents inlining of such code.
>> +    // See also the comments to visitIndirectBrInst() and in analyzeCall().
>> +    if (isa<IndirectBrInst>(BI->getTerminator()) || BI->hasAddressTaken())
>>       return false;
>> 
>>     for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;
>> 
>> 
>> This comment seems a little verbose to me. I would at least remove the
>> cross-references to comments in other functions.
>> 
>>