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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> My apologies that I did not try cfi-inserter-basic-block-sections-callee-save-registers.ll
>  The generated assembly does have .cfi_offset
>  (I cannot apply the patch with `arc patch` (https://reviews.llvm.org/D79978?id=277216#2138208 )
>  so I used curl | patch and somehow ignored the locally modified .ll)
>
> However, I do think inline asm with clobbered register list () has some advantage: the .cfi_offset registers
>  can be precisely controlled.
>
>   call void asm sideeffect "nop", "~{rbp},~{r12},~{r13},~{r14},~{r15}"()
>   
>
> The CHECK lines can thus be strengthened:
>
>   ; CFI_INSTR:      CFI_INSTRUCTION def_cfa_offset 48
>   ; CFI_INSTR-NEXT: CFI_INSTRUCTION offset $r12, -48
>   ; CFI_INSTR-NEXT: CFI_INSTRUCTION offset $r13, -40
>   ; CFI_INSTR-NEXT: CFI_INSTRUCTION offset $r14, -32
>   ; CFI_INSTR-NEXT: CFI_INSTRUCTION offset $r15, -24
>   ; CFI_INSTR-NEXT: CFI_INSTRUCTION offset $rbp, -16
>
>
> While the current approach cannot control the used registers.


Yeah, I have mixed feelings about this - not sure it's necessary to test all of these, or test them as strongly as you've suggested (overly constraining tests can make them brittle - prone to breakage for other reasons (eg: it might still be worth leaving off the specific stack offset, so that if stack layout changes these tests don't fail - other, pre-existing, tests specifically for stack layout should catch that)). And usually I'd advocate for not using inline assembly (as in the other test). But in this case, given how indirect the code is when trying to clobber the callee saved register - yeah, I think inline asm might've been more clear to me in that test. Though I think my non-asm suggestion (https://reviews.llvm.org/D79978#2145753) did manage to reduce the test down to exactly one register in a fairly understandable way... hmm, could be a bit simpler now that I understand better what's going on:

  void clobber();
  void sink(int);
  void test(bool b, int i) {
    if (b) { // adds a basic block
      clobber(); // encourage 'i' to be in a callee-save register by clobbering caller-save registers
      sink(i); // keeps 'i' alive
    }
  }

So that tests one callee save register, assuming LLVM doesn't decide to (gratuitously inefficiently - well, I guess it might not be that much less efficient) put 'i' on the stack or somewhere else instead.

Whereas @maskray's suggestion of using explicit register use/clobber does seem a bit more explicit/intentional, rather than having to coax a certain representation out of the compiler backend, I guess something like:

  void f3(bool b) { 
    if (b) // adds a basic block
       // clobber some example callee-save registers to force them to be callee-saved and to be described by cfi_offset directives
      asm("nop" : : : "r12","r13","r14","r15");
  }

(at least I'm not sure much else is required - @MaskRay - was the second inline asm needed? or the return values/parameters? Also perhaps just testing one register would be sufficient? (less ordering issues, chance for unrelated backend changes to disturb this test))



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3087-3091
+  if (MBB.isEndSection()) {
+    for (const HandlerInfo &HI : Handlers) {
+      HI.Handler->endBasicBlock(MBB);
+    }
+  }
----------------
probably drop the braces here too (as above)


================
Comment at: llvm/test/CodeGen/X86/cfi-basic-block-sections-1.ll:3
+; RUN: llc -O0 %s --basicblock-sections=all -mtriple=x86_64-unknown-linux-gnu -filetype=asm --frame-pointer=none -o - | FileCheck --check-prefix=SECTIONS_NOFP_CFI %s
+; RUN: llc -O0 %s --basicblock-sections=all -mtriple=x86_64-unknown-linux-gnu -filetype=obj --frame-pointer=all -o - | llvm-dwarfdump --debug-frame  - | FileCheck --check-prefix=DEBUG_FRAME %s
+
----------------
MaskRay wrote:
> While `--eh-frame` is an alias for `--debug-frame`, I think using `--eh-frame` here is more appropriate. This tests .eh_frame, not .debug_frame.
> 
Agreed - the check on line 51 should be updated too. llvm-dwarfdump's output is actually rendering an empty debug_frame and then after that it's rendering the eh_frame - the test is currently a bit misleading down there too. (& maybe llvm-dwarfdump's output is prone to such misleading testing, unfortunately - it prints the name of every section explicitly requested (& considers eh_frame and debug_frame explicitly requested when using --eh-frame or --debug-frame) even if they're empty, so you can easily get this sort of flow-on effect)


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

https://reviews.llvm.org/D79978