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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D79978#2146009 <https://reviews.llvm.org/D79978#2146009>, @amharc wrote:

> In D79978#2145979 <https://reviews.llvm.org/D79978#2145979>, @MaskRay wrote:
>
> > 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?
>
>
> I'm not exactly sure which CFI instruction this comment refers to (there is no `.cfa_offset` directive, see e.g. https://sourceware.org/binutils/docs/as/CFI-directives.html).
>
> If to `.cfi_def_cfa_offset` - the question was answered before: we emit a full CFA definition (`.cfi_def_cfa` - setting both the offset and the register) which is equivalent to a `.cfi_def_cfa_offset` (only setting the offset) plus a `.cfi_def_cfa_register` (only setting the register). Moreover, for code following the SysV ABI the register used in the `.cfi_def_cfa_register`/`.cfi_def_cfa` instructions is conventionally always set to either `rbp` (if the frame pointer is kept explicitly) or in `rsp` (when it's omitted). Both cases are covered by the existing tests. We are not aware of any easy way to force a different register to be the frame pointer.
>
> If the review comment above refers to `.cfi_offset` instead, please note that the `cfi-basic-block-sections-1.ll` test checks that `.cfi_offset` is emitted properly in the generated assembly and `cfi-inserter-basic-block-sections-callee-save-registers.ll` checks that the CFI Inserter Pass actually inserts the corresponding `CFI_INSTRUCTION offset` instructions for clobbered callee-saved registers. I'm not exactly sure what would be gained by using inline assembly instead.


I ran llc for both tests. Neither has a section with CFI describing a non-rbp register at the top, thus I say I am not sure the implementation is properly tested. My proposed inline assembly makes sure more than one registers (not just the current CFA register) are recorded. Please inspect the output.


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

https://reviews.llvm.org/D79978