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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@212077 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Analysis/IPA/InlineCost.cpp b/lib/Analysis/IPA/InlineCost.cpp
index 66f3f8e02528df1159483b885c159b2abd45e557..8807529cabac1ba44607fd5645e81eb1eb0e1425 100644 (file)
--- a/lib/Analysis/IPA/InlineCost.cpp
+++ b/lib/Analysis/IPA/InlineCost.cpp
@@ -841,10 +841,7 @@ bool CallAnalyzer::visitIndirectBrInst(IndirectBrInst &IBI) {
   // 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 CS) {
     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(Function &F) {
     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;

diff --git a/test/Transforms/Inline/blockaddress.ll b/test/Transforms/Inline/blockaddress.ll
index 4206312d77436a35ded2d57e8547e1ee57181c4f..8eb307250330125ab8424376dd9264193b7e1400 100644 (file)
--- a/test/Transforms/Inline/blockaddress.ll
+++ b/test/Transforms/Inline/blockaddress.ll
@@ -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