[PATCH] D76519: [NFC] Refactor how CFI move sections are represented in AsmPrinter

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a subscriber: ostannard.
dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:974
 
-AsmPrinter::CFIMoveType AsmPrinter::needsCFIMoves() const {
+unsigned AsmPrinter::getCFIMoveTypes(const Function &F) const {
+  // Ignore functions that won't get emitted.
----------------
scott.linder wrote:
> dblaikie wrote:
> > Naively, it looks to me like this API change would allow for emission of debug_frame and eh_frame simultaneously. Is that understanding correct? If it is, is that outcome useful/intended? I'd have thought one or the other would suffice? (if you have eh_frame and its drawback of being ALLOC - you don't gain anything by also emmitting debug_frame, do you?)
> Yes, that's correct. It is still NFC, as that was the behavior before, it just wasn't represented here. See below:
> 
> ```
> else if (Asm->TM.Options.ForceDwarfFrameSection)
>       Asm->OutStreamer->emitCFISections(true, true);
> ```
> 
> IIRC this was the confusion that led me to the refactoring to begin with. It also generalizes `isCFIMoveForDebugging` and documents what it actually is.
> 
> As to whether it makes sense to emit both, I don't know, but it is intended (emphasis mine):
> 
> >>! In D67216#1705839, @dcandler wrote:
> > I've modified the patch so that the new flag will ensure the cfi instructions are actually present to be emitted as well. I went ahead and renamed the flag -gdwarf-frame too, to better reflect that it's dealing with the debug information you'd otherwise get with -g, and is meant to specifically put the information in a .debug_frame section and not .eh_frame.
> > 
> > Currently, two things signal for need for cfi: exceptions (via the function's needsUnwindTableEntry()), and debug (via the machine module information's hasDebugInfo()). At frame lowering, both trigger the same thing. But when the assembly printer decides on which section to use, needsUnwindTableEntry() is checked first and triggers the need for .eh_frame, while hasDebugInfo() is checked afterwards for whether .debug_frame is needed. So .debug_frame is only present when any level of debug is requested, and no functions need unwinding for exceptions.
> > 
> > It wouldn't be appropriate to change either needsUnwindTableEntry() or hasDebugInfo(), so I've added a check for my flag alongside them. Because the same logic is used in multiple places, I've wrapped all three checks into one function to try and clean things up slightly. **When deciding on which section to emit, the new flag means .debug_frame is produced instead of nothing. If .eh_frame would have been needed, rather than replace it, the new flag simply emits both .debug_frame and .eh_frame.**
> > 
> > The end result is that -gdwarf-frame should only provide a .debug_frame section as additional information, without otherwise modifying anything. The existing -funwind-tables (and -fasynchronous-unwind-tables) flag can be used to provide similar information, but because it takes the exception angle, it alters function attributes and ultimately produces .eh_frame instead.
Sorry, I don't have enough context here, so it's still a bit confusing:

"It is still NFC, as that was the behavior before, it just wasn't represented here. " - you're saying that prior to this patch/currently in LLVM, it was possible that both eh_frame and debug_frame were emitted into the same output file? (is that a useful thing? why is that supported?)

@dcandler @ostannard - do you folks have some context you could share here?

But, at least as far as the immediate/blocking issues for this current proposed patch are concerned - yeah, I guess this API change clarifies the existing behavior/intent. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76519