[llvm] r212077 - Suppress inlining when the block address is taken

[Gerolf Hoflehner]

Author: ghoflehner
Date: Mon Jun 30 19:19:34 2014
New Revision: 212077

URL: http://llvm.org/viewvc/llvm-project?rev=212077&view=rev
Log:
Suppress inlining when the block address is taken

Inlining functions with block addresses 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.

Background:
There have been reports on Ruby segmentation faults triggered by inlining
functions with block addresses 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.

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 was 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 had it worked 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.

Test case ./tools/clang/test/CodeGen/indirect-goto.c will fail as a result
of this commit.

rdar://17245966


Modified:
    llvm/trunk/lib/Analysis/IPA/InlineCost.cpp
    llvm/trunk/test/Transforms/Inline/blockaddress.ll

Modified: llvm/trunk/lib/Analysis/IPA/InlineCost.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/IPA/InlineCost.cpp?rev=212077&r1=212076&r2=212077&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/IPA/InlineCost.cpp (original)
+++ llvm/trunk/lib/Analysis/IPA/InlineCost.cpp Mon Jun 30 19:19:34 2014
@@ -841,10 +841,7 @@ bool CallAnalyzer::visitIndirectBrInst(I
   // original function which is extremely undefined behavior.
   // FIXME: This logic isn't really right; we can safely inline functions with
   // indirectbr's as long as no other function or global references the
-  // blockaddress of a block within the current function.  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.
+  // blockaddress of a block within the current function.
   HasIndirectBr = true;
   return false;
 }
@@ -1121,6 +1118,15 @@ bool CallAnalyzer::analyzeCall(CallSite
     if (BB->empty())
       continue;
 
+    // Disallow inlining a blockaddress. A blockaddress only has defined
+    // behavior for an indirect branch in the same function, and we do not
+    // currently support inlining indirect branches. But, the inliner may not
+    // see an indirect branch that ends up being dead code at a particular call
+    // site. If the blockaddress escapes the function, e.g., via a global
+    // variable, inlining may lead to an invalid cross-function reference.
+    if (BB->hasAddressTaken())
+      return false;
+
     // Analyze the cost of this block. If we blow through the threshold, this
     // returns false, and we can bail on out.
     if (!analyzeBlock(BB)) {
@@ -1303,8 +1309,9 @@ bool InlineCostAnalysis::isInlineViable(
     F.getAttributes().hasAttribute(AttributeSet::FunctionIndex,
                                    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()))
+    // Disallow inlining of functions which contain indirect branches or
+    // blockaddresses.
+    if (isa<IndirectBrInst>(BI->getTerminator()) || BI->hasAddressTaken())
       return false;
 
     for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;

Modified: llvm/trunk/test/Transforms/Inline/blockaddress.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/Inline/blockaddress.ll?rev=212077&r1=212076&r2=212077&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/Inline/blockaddress.ll (original)
+++ llvm/trunk/test/Transforms/Inline/blockaddress.ll Mon Jun 30 19:19:34 2014
@@ -1,8 +1,9 @@
 ; RUN: opt -inline -S < %s | FileCheck %s
 ; PR10162
 
-; Make sure the blockaddress is mapped correctly when doit is inlined
-; CHECK: store i8* blockaddress(@f, %here.i), i8** @ptr1, align 8
+; Make sure doit is not inlined since the blockaddress is taken
+; which could be unsafe
+; CHECK: store i8* blockaddress(@doit, %here), i8** %pptr, align 8
 
 @i = global i32 1, align 4
 @ptr1 = common global i8* null, align 8