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

[Eric Christopher]

Yup. Thanks for the history there. I agree it shouldn't be inlined.
On Jun 25, 2014 3:17 PM, "Bob Wilson" <bob.wilson at apple.com> wrote:

>
> > 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.
> >>
> >>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140625/147445ef/attachment.html>