fix for Ruby build fails due to inlining functions with block label address taken
Bob Wilson
bob.wilson at apple.com
Mon Jun 30 22:04:59 PDT 2014
I disabled the test in clang r212091.
Gerolf, please don’t ever leave tests failing like that.
> On Jun 30, 2014, at 6:07 PM, Eric Christopher <echristo at gmail.com> wrote:
>
> On Mon, Jun 30, 2014 at 6:00 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>> Thanks. You also need to commit a change to clang’s indirect-goto test,
>> right? Otherwise, it’s going to start failing now.
>
> s/start failing/continue to fail :)
>
> -eric
>
>>
>> On Jun 30, 2014, at 5:41 PM, Gerolf Hoflehner <ghoflehner at apple.com> wrote:
>>
>> 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.
>>
>>
More information about the llvm-commits
mailing list