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

[Eric Christopher]

On Wed, Jun 25, 2014 at 3:23 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
>> 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.
>

Aha. No issue then. :)

Thanks Bob.

-eric

>>
>> 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.
>>>
>>>
>>>
>