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

[Bob Wilson]

> On Jun 25, 2014, at 2:49 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
> On Wed, Jun 25, 2014 at 2:46 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>> On 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?
>> 
>> 
>> 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.
>> 
>> 
> 
> Oh, wait, I see what the testcase is doing now. I'd thought it was
> inlining into something that had blockaddr rather than the other way
> around.
> 
> What does the commit for the testcase say about it?

It appears to be a new testcase that Gerolf created to demonstrate the issue he was asking about. I went looking for the commit and couldn’t find it anywhere.

> 
> Assuming it doesn't say anything enlightening then no, no issue here either.
> 
> -eric
> 
>> 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.
>> 
>> 
>>