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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

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

> In D79978#2141992 <https://reviews.llvm.org/D79978#2141992>, @wmi wrote:
>
> > In D79978#2141959 <https://reviews.llvm.org/D79978#2141959>, @MaskRay wrote:
> >
> > > Ack. Then what instructions should be placed at the top of these basic blocks? Should `.cfi_def_cfa_register %rbp` be placed as well? If you move these basic blocks around, `.cfi_def_cfa_register %rbp` is currently not tracked.
> >
> >
> > That is because .cfi_def_cfa %rbp, 16 is identical to the following:
> >  .cfi_def_cfa_register %rbp
> >  .cfi_def_cfa_offset 16
>
>
> Honestly I am not a CFI expert but I have read enough bits of LLVM libunwind and am not completely CFI illiterate (I have fixed a very subtle negative cfiDefCfa bug). The description of the patch is still puzzling me.


I am not a CFI expert either and that is not a problem.

> I think it lacks a summary about what the patch intends to do.

Ok, I can write a more detailed summary here.

> Is the intention: if the entry block stays in the front of the function, while other basic blocks can be randomly shuffled => the CFI states of all basic blocks are the same no matter how non-entry basic blocks are shuffled?

@amharc  If a basic block is placed in a unique section then it can potentially be moved away from the original function.   CIEs do not allow ranges unlike debug info.  So, when you are the PC of this basic block how does the unwinder know where the CFA is?  In order to do that, we have to replicate the cfi directives to say how to find this.  This test you pointed out shows that we do this with cfi directives.

Please note that the control-flow of the program never changes even though the blocks are shuffled randomly.

> Or
> 
> The entry basic block can be shuffled as well?

The entry basic block is fine as it has the symbol of the original function and the default CFI generated is correct.  Its section is the same as the function's section.

> For either behavior, I am not sure the test covers the situations: I can only find `.cfi_def_cfa_register` in the entry block, not in others - so I am not confident that `.cfi_def_cfa_register` information is correctly retained. We need a stronger test.

Sure, could you please tell us what  we should be testing.  We have a test for callee saved register cfi directives being generated too.

> For convenience of other reviewers, the diff when -basicblock-sections=all is turned on:
> 
>   -       je      .LBB0_2                                                                                                                               
>   -# %bb.1:                                # %if.then                                                                                                   
>   +       je      _Z2f3b.2                                                                                                                              
>   +       jmp     _Z2f3b.1                                                                                                                              
>   +       .cfi_endproc                                                                                                                                  
>   +       .section        .text,"ax", at progbits,unique,1                                                                                                 
>   +_Z2f3b.1:                               # %if.then                                                                                                   
>   +       .cfi_startproc                                                                                                                                
>   +       .cfi_def_cfa %rbp, 16                                                                                                                         
>   +       .cfi_offset %rbp, -16
>           callq   _Z2f1v
>   -.LBB0_2:                                # %if.end
>   +       jmp     _Z2f3b.2
>   +.Ltmp0:
>   +       .size   _Z2f3b.1, .Ltmp0-_Z2f3b.1
>   +       .cfi_endproc
>   +       .section        .text,"ax", at progbits,unique,2
>   +_Z2f3b.2:                               # %if.end
>   +       .cfi_startproc
>   +       .cfi_def_cfa %rbp, 16
>   +       .cfi_offset %rbp, -16
>           addq    $16, %rsp
>           popq    %rbp
>           .cfi_def_cfa %rsp, 8
>           retq
>   
> 
> 
> Note also that only `rbp` is described. I think we need another register to demonstrate the effect.

The other test does check for callee saved registers. This test is not only to check %rbp but also to make sure cfi_startproc and cfi_endproc are generated as expected.


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

https://reviews.llvm.org/D79978