fix for Ruby build fails due to inlining functions with block label address taken
Eric Christopher
echristo at gmail.com
Mon Jun 30 15:32:45 PDT 2014
On Mon, Jun 30, 2014 at 3:31 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> On Jun 30, 2014, at 3:21 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> On Mon, Jun 30, 2014 at 3:18 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
>
> On Jun 27, 2014, at 4:26 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>
> I revised the comments and commit message of the Ruby fix.
>
>
> <0001-Suppress-inlining-when-the-block-address-is-taken.patch>
>
>
> Thanks. This looks close. Please go ahead and commit with fixes for the
> following:
>
> @@ -1097,6 +1094,14 @@ 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
> an\
> d
> + // only take a subset of the blocks into account there may not be an
> + // indirect branch. Such code does not make much sense since a block
> + // address label is allowed to be a jump target only within a function.
> + // Worse, in one instance it caused a segmentation fault in a Ruby
> build.
> + 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.
>
>
> You made some other changes since the last revision. Please put the return
> on a separate line, like you had it before.
>
> I’m still not happy with the comment. The last part does not provide enough
> information to understand _why_ it caused the Ruby segfault, so it’s not
> really adding any value. It’s also not clear what you mean here by
> “partially inline”. Would you be OK to replace it with this?
>
> “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."
>
>
> This comment is absolutely ideal. :)
>
> Might want to add to the comment with why we can't support it right now?
>
>
> That seems like it would be more appropriate in lib/Analysis/README.txt, as
> a suggestion for future work.
>
I'm down with that. I just prefer things in the comments if at all
possible and figured if it were short...
>
> @@ -1278,8 +1283,15 @@ 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 an indirect branch.
> 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 possibly be harmful (e.g. when the address is assigned a
> global
> + // variable which is used in a global (cross-funtion) goto).
> + if (isa<IndirectBrInst>(BI->getTerminator()) || BI->hasAddressTaken())
> return false;
>
> for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;
>
>
> I still find this comment to be less than helpful. The problem you describe
> here has nothing to do with whether inlining is viable. You’re saying that
> we should not inline functions with invalid code. Why does inlining make it
> any worse? I’m not opposed to adding the check for hasAddressTaken(), but I
> would prefer to just have the comment say: "Disallow inlining of functions
> which contain indirect branches or blockaddresses."
>
>
> Agree, might want to reference the longer comment above or just copy it?
>
> -eric
>
>
> I’d really like to avoid copying it. The situation in the longer comment
> does not apply here. This is just a QOI issue to avoid crazy stuff in the
> face of invalid code.
>
>
Sounds good.
-eric
>
> I continue to work on the indirect-goto.c case. The fix that would make it
> happen is inverting the
> order of invocation of global const prop and global optimizer:
>
> Index: lib/Transforms/IPO/PassManagerBuilder.cpp
> ===================================================================
> --- lib/Transforms/IPO/PassManagerBuilder.cpp (revision 211729)
> +++ lib/Transforms/IPO/PassManagerBuilder.cpp (working copy)
> @@ -156,9 +156,9 @@
> if (!DisableUnitAtATime) {
> addExtensionsToPM(EP_ModuleOptimizerEarly, MPM);
>
> - MPM.add(createGlobalOptimizerPass()); // Optimize out global vars
>
> MPM.add(createIPSCCPPass()); // IP SCCP
> + MPM.add(createGlobalOptimizerPass()); // Optimize out global vars
> MPM.add(createDeadArgEliminationPass()); // Dead argument elimination
>
> MPM.add(createInstructionCombiningPass());// Clean up after IPCP & DAE
>
>
> When the global optimizer is invoked first, the indirect branch using the
> global array has not
> been eliminated. So the global can’t be eliminated. That is pretty straight
> forward and the pass swap
> looks reasonable but requires more scrutiny and careful testing. As
> additional benefit the global
> array would be removed from the IR.
>
>
> -Gerolf
>
> On Jun 25, 2014, at 6:01 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>
> Bob and I discussed this a bit more. Interprocedural const prop eliminates
> the indirect branch. Then the labels. So when the
> inliner kicks in it does not “see" any of the issues that could block it.
>
> However, with the patch there must still be some information about block
> label address taken kept around when a
> global (static) array contains the label addresses. I verified that the
> test case would work when foo2
> also declared just a local array const void *addrs[] = { &&L1, &&L2, &&L3,
> &&L4, &&L5 }.
> Optimistically it seems like we might catch the global case also without
> escape analysis,
> but I have not looked into that code yet.
>
> -Gerolf
>
>
>
> On Jun 25, 2014, at 4:00 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>
> Oh! It’s a front-end test. I didn’t even think to look there….
>
> The test predates the introduction of indirect branches and has only had
> superficial changes since then. We used to translate computed gotos into
> switch statements. I’m still trying to figure out how this ever worked in
> the past.
>
> On Jun 25, 2014, at 3:50 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>
> Looking at the history it looks like Chris L. committed the test case in
> tools/clang/test/CodeGen/indirect-goto.c to test
> block address labels.
>
> I’m fine with the train of reasoning:
> there is an indirect branch in the source and we
> never attempt to inline such functions. Furthermore for inlining functions
> with block
> label address taken we would need escape analysis. We don’t have it. So
> let’s not inline.
>
> It looks like everyone is signing up for
> “Expect no optimization when block address labels are used”.
>
> Cheers
> Gerolf
>
>
>
>
> 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?
>
> 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.
>
>
More information about the llvm-commits
mailing list