[llvm] r207713 - Patch for function cloning to inline all blocks whose address is taken

[Gerolf Hoflehner]

Yikes, this completely escaped me. Shame on me!

Eric, thanks for jumping in and resolving the issue for Joerg in the short term.  Looking at the problem, however, I think it is best to change to the source code by adding a no-inline attribute to vm_exec_core(),  and fix more issues in the cloner.

We had some discussions why the compiler shouldn’t inline functions when it contains indirect branches and address of its labels are taken  - at least unless there is analysis in place that determines it is safe to do so. Well, the issue here is related to that discussion:  vm_exec_core(), where the address of label INSN_LABEL_finish is taken and assigned to a global variable, is inlined into rb_vm_get_insn. In the “good” the caller (rb_vm_get_insns_address_table()) takes the address of that label in vm_exec_core() [!!] and stores it into a global.

; Function Attrs: nounwind ssp uwtable
 define noalias i32* @rb_vm_get_insns_address_table() #0 {
 entry:
   store i32 ptrtoint (i8* blockaddress(@vm_exec_core, %INSN_LABEL_finish) to i32), i32* @fi     nish_insn_seq_0, align 4, !tbaa !1
   ret i32* undef
 }

define i32 @vm_exec_core() #0 {
entry:
  store i32 ptrtoint (i8* blockaddress(@vm_exec_core, %INSN_LABEL_finish) to i32), i32* @fi     nish_insn_seq_0, align 4, !tbaa !1
  br label %INSN_LABEL_finish
INSN_LABEL_finish:                                ; preds = %entry
  ret i32 undef


With my patch the label became part of the callee (as it should be):

  store i32 ptrtoint (i8* blockaddress(@rb_vm_get_insns_address_table, %INSN_LABEL_finish.i     ) to i32), i32* @finish_insn_seq_0, align 4, !tbaa !1
  br label %INSN_LABEL_finish.i

But then (still in the cloner) the branch is eliminated, the label removed and the final code reads:

define i32* @rb_vm_get_insns_address_table() #0 {
entry:
  store i32 1, i32* @finish_insn_seq_0, align 4, !tbaa !1  // The jump to address 1 causes the seqfault
  ret i32* undef
}

Conclusion:
1) I suggest adding the no-inline attribute to vm_exec_core() to be on the safe side. Note the actual code may not segfault, but could be incorrect. 
2) Reapply the patch
3) Suppress inlining when the address of a label is taken (until there is analysis in place that determines inlining is safe). See previous discussion in context with indirect branches. 
4) CloneFunction.cpp should not optimize away labels whose address is taken (another bug lurking in this code). Note that outside the cloner there is no issue since the label in vm_exec_core() is not touched.

With the patch just taking care of 4) (and ignoring 3))  is probably enough since the label becomes part of the caller.

What do you think?

Cheers
Gerolf


On May 19, 2014, at 9:15 AM, Joerg Sonnenberger <joerg at britannica.bec.de> wrote:

> On Mon, May 19, 2014 at 09:11:46AM -0700, Eric Christopher wrote:
>> Hi Gerolf,
>> 
>> I've gone ahead and reverted this in r209135.
>> 
>> Joerg: What does Gerolf need to do other than compile the files? Link
>> them? Compile them together? A set of command lines might be good.
> 
> Compile them together and run it. The broken version will hit the
> segfault handler.
> 
> Joerg
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140619/1e984382/attachment.html>