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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

In D79978#2146366 <https://reviews.llvm.org/D79978#2146366>, @dblaikie wrote:

> In D79978#2146131 <https://reviews.llvm.org/D79978#2146131>, @tmsriram wrote:
>
> > Let me try a slightly different approach here.  It is not clear to us what more is needed to land the patch.  In the interests of resolving conflict :
> >
> > 1. I will also explicitly test assembly too for callee saved registers with bb sections when they are being pushed and popped.
>
>
> Just on this point - it's not clear to me the motivation for the difference in testing between the two tests (one testing assembly, the other testing LLVM's MIR) - was there some particular difference/detail that these different strategies enabled testing? Otherwise, yeah, I'd probably just go with only testing assembly in both cases, for consistency/simplicity? (any variation in tests always makes me wonder "what's the reason for this difference? Must be something important or they'd be the same and so I'm not understanding some important difference")


You are right.  For reasons that are long forgotten since this was written a while go, one of us thought it would be good to directly check CFI IR instructions too.  Like you note, we could have just tested assembly, changed now.



================
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
+
----------------
dblaikie wrote:
> 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)
Fixed this,  stems from my mis-understanding of the various tools output.


================
Comment at: llvm/test/CodeGen/X86/cfi-inserter-basic-block-sections-callee-save-registers.ll:21-36
+; Exhaust caller-saved parameter registers and  force callee saved registers to
+; be used.  This tests that CFI directives for callee saved registers are
+; generated with basic block sections.
+; extern void f1(int, int, int);
+;
+; void foo(bool k, int p1, int p2, int p3, int p4, int p5, int p6) {
+;   // Using a conditional forces a basic block section.
----------------
dblaikie wrote:
> Thanks for this - totally makes sense to me now!
> (Totally optional: I'd probably just write this with one parameter? And/or perhaps with constants for the first f1 call? - I've needed to do things like this when testing the DWARF OP_entry_values:
> 
> ```
> void f1(int);
> extern bool b;
> void f3(int i) {
>   if (b) { // adds a basic block
>     f1(1); // clobbers 'i', causing it to be spilled
>     f1(i); // keeps 'i' alive
>   }
> }
> ```
> 
> Not to nitpick - just trying to help ensure tests are very specifically targeted - at least makes them easier for me to read, not sure about other folks? (if there's something interesting about testing more than one of these - some comments describing that'd be handy too)
> 
> What you've got is fine though, if you prefer it/find it valuable
> 
> Thanks for the general write-up of CFI and this patch - helped me understand a bit of what this patch in general, and this test in particular are all about!)
Marking it done since you wanted to go with your other suggestion with inline asm.


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

https://reviews.llvm.org/D79978