fix for Ruby build fails due to inlining functions with block label address taken
Gerolf Hoflehner
ghoflehner at apple.com
Mon Jun 30 17:41:41 PDT 2014
Thanks, Bob. Committed revision 212077. Please see below.
On 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.
Yikes, when I forced the 80 character alignment it moved the return too…
>
> 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.”
Happy to steel that treasure of a comment for the benefit of the community :-)
>
>> @@ -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 block addresses.”
I like the brevity of your suggestion and went with it.
>
>>
>> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140630/23dfa71b/attachment.html>
More information about the llvm-commits
mailing list