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

[Eric Christopher]

Indirect goto is pretty important IIRC for threaded interpreters.
What's going on that we're not optimizing it anymore?

-eric

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