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

[Krzysztof Pszeniczny via Phabricator via llvm-commits]

amharc added a comment.

Ple

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 think it lacks a summary about what the patch intends to do.
>
> 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?
>
> Or
>
> The entry basic block can be shuffled as well?


We explicitly want to support the case where all BBs can be shuffled around arbitrarily.

> 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.

It is retained, because we always emit a full `.cfi_def_cfa` in all non-entry basic blocks - which, as @wmi has mentioned before, is equivalent to a `.cfi_def_cfa_register` and a `.cfi_def_cfa_offset`. For non-entry basic blocks, there is no previous state (neither the cfa register nor the cfa offset), so we cannot emit just `.cfi_def_cfa_register` or just `.cfi_def_cfa_offset`, because both of them preserve one part of the cfa definition (the offset for `.cfi_def_cfa_register` or the register for `.cfi_def_cfa_offset`).

> 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.


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

https://reviews.llvm.org/D79978