[PATCH] D79978: Call Frame Information (CFI) Handling for Basic Block Sections

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

In D79978#2145979 <https://reviews.llvm.org/D79978#2145979>, @MaskRay wrote:

> In D79978#2145413 <https://reviews.llvm.org/D79978#2145413>, @tmsriram wrote:
>
> > ...
> >  Now, what happens when we start placing individual basic blocks in unique sections:
> >
> > - Basic block sections allow the linker to randomly reorder basic blocks in the address space such that a given basic block can become non-contiguous with the original function.
> > - The different basic block sections can no longer share the cfi_startproc and cfi_endproc directives.  So, each basic block section should emit this independently.
> > - Each (cfi_startproc, cfi_endproc) directive will result in a new FDE that caters to that basic block section.
> > - Now, this basic block section needs to duplicate the information from the entry block to compute the CFA as it is an independent entity.  It cannot refer to the FDE of the original function and hence must duplicate all the stuff that is needed to compute the CFA on its own. ...
>
>
> Thanks for the detailed write-up! I I've learned a bit from it. I think at least the quoted items should be placed in the description.
>
> In D79978#2145189 <https://reviews.llvm.org/D79978#2145189>, @amharc wrote:
>
> > > Note also that only `rbp` is described. I think we need another register to demonstrate the effect.
> >
> > `rbp` is the usual frame pointer register for the x86 architecture and I'm not really sure we can easily force the compiler to choose a different register to hold the frame pointer. If you know how to force a different register to be the frame pointer, please let us know - we will add a corresponding test.
>
>
> I cannot find a test with `.cfa_offset` or describing a non-rbp register, so the implementation is under-tested. How about leveraging inline asm to clobber callee-saved registers?


Please feel free to double-check with any body familiar with X86 ,  X86 has exactly one register for the frame pointer and that is %rbp, there is *no other register*.    The only other option is to omit the frame pointer and *we have checked both now explicitly*.

We are already checking for callee-saved registers.  That is the whole point of test 2 , there is no need to force inline assembly.  @dblaikie could you please help explain here?  If you want us to check the final assembly too we can, but I don't see why you are asking for inline assembly when a program could do this.

> 
> 
>   int f3(int i, int j, int k) {
>     if (i == 0) { // adds a basic block
>       asm("nop" : : : "rdi","rsi","rdx","rbp","r12","r13","r14","r15","memory"); // there is a .cfi_offset for each of rbp,r12,r13,r14,r15
>       return j;
>     }
>     if (j == 0) { // adds a basic block
>       asm("xchg %%ax,%%ax" : : : "rdi","rsi","rdx","rbp","r14","r15","memory");  // r12 and r13 are not clobbered but the current implementation adds .cfi_offset for both r12 and r13
>       return k;
>     }
>     return i;
>   }
> 
> 
> I get a lot of `.cfi_offset` with `clang -S -emit-llvm -O1 a.c; llc -O0 -basicblock-sections=all < a.ll`:
> 
>   ...
>           .section        .text,"ax", at progbits,unique,1
>   f3.1:                                   # %if.then
>           .cfi_startproc
>           .cfi_def_cfa %rsp, 48
>           .cfi_offset %r12, -48
>           .cfi_offset %r13, -40
>           .cfi_offset %r14, -32
>           .cfi_offset %r15, -24
>           .cfi_offset %rbp, -16

It is the same thing, it is exactly what test2 does.  Do you want use to test the assembly too?  We can do it without inline assembly,  I personally hate unnecessary uses of inline assembly.

>   #APP
>   nop
>   #NO_APP
>   movl    -8(%rsp), %eax                  # 4-byte Reload
>   movl    %eax, -16(%rsp)                 # 4-byte Spill
> 
> ...
> 
>   




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79978/new/

https://reviews.llvm.org/D79978