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

Sat Apr 24 08:22:58 PDT 2021

RamNalamothu added a subscriber: t-tye.
RamNalamothu marked 2 inline comments as done.
RamNalamothu added a comment.

In D76519#2714352 <https://reviews.llvm.org/D76519#2714352>, @MaskRay wrote:

>> Refactor how CFI move sections
>
> Since 'move' is coined term which is not accurate, just avoid using it.

Ah, I had fixed it in the commit message but not on Phabricator.

> Can you rephrase why we need both .eh_frame & .debug_frame?

Capability of emitting both sections was already there and this patch just maintained the status quo.

Probably the following was adding the confusion. I have changed it little bit in the latest diff update.

  /// Cached bitwise OR of CFISection flags for all functions in the module.
  unsigned ModuleCFISections = CFISection::None;//

And on why emit both sections, as it was mentioned earlier also, this is all that comes to my mind.

> In D67216#1705839 <https://reviews.llvm.org/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.

> @scott.linder wrote:
> The first thing that comes to my mind is if you have a language runtime which requires `.eh_frame` (i.e. for EH) and some other tool which only understands `.debug_frame`. The runtime cannot use `.debug_frame` (or I suppose it could if it were marked ALLOC and the runtime were willing to do some more relocation), and the tool would have to be taught how to use `.eh_frame` (this seems reasonable, but so does just putting in both tables). It would be convenient in that case to just emit both.

> @RamNalamothu wrote:
> Yes, both `.eh_frame` and `.debug_frame` can be emitted into the same output file. AFAIU this ability is primarily needed for the following reasons.
>
> 1. PowerPC uses different register numbering in `.debug_frame` and `.eh_frame`. I think, this is (one of) the reason why GCC also has the capability to  generate both `.eh_frame` and `.debug_frame` into the same output file.
>
> 2. Erstwhile LLVM toolchain used the system assembler/linker, typically the GCC's assembler/linker, and also I think the LLVM's assembler/linker strives to be a drop in replacement for GCC ones. And this naturally forces LLVM to support GAS assembler directives and their syntax.
>
> 3. A strict DWARF compliant debugger needs `.debug_frame` which doesn't have exception handling features and this forces generating `.eh_frame` as well if we also need exception handling support in those cases.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76519