[PATCH] D78778: [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo

Wed Dec 9 18:33:28 PST 2020

dblaikie added a comment.

(I'm probably not the best person to review this - just chimed in on the patch structure/testing issues, but here are some naive questions)

In D78778#2301155 <https://reviews.llvm.org/D78778#2301155>, @RamNalamothu wrote:

> In D78778#2300827 <https://reviews.llvm.org/D78778#2300827>, @probinson wrote:
>
>> I haven't touched the streamer side of AsmPrinter in a long time, but...
>> On PS4 we default to `-fno-exceptions` and while the section ends up named `.eh_frame` instead of `.debug_frame` the content is fine; we've had no complaints from our debugger folks.  Aside from the introduction of the .cfi directives (which you do want), I'm not aware that there's any codegen consequence to this combination.
>> So, I'm not clear why a special streamer is really necessary.
>
> A special streamer is a straight forward and minimal changes approach we could arrive at as a means to solve the following concerns observed with the current implementation.
>
> 1. Today, with //`-fno-exceptions`//, the command line option //`--force-dwarf-frame-section`// of llc doesn't do what it should as per D67216 <https://reviews.llvm.org/D67216> i.e. a  //`.debug_frame`// is not generated

Does this change address (1)? Should there be a test for `--force-dwarf-frame-section` to demonstrate this fix? (what cases did D67216 <https://reviews.llvm.org/D67216> address? that are distinct from this case that it misses)

> 2. The //`.eh_frame`// needs to be an `ALLOC` section and generating //`.eh_frame`// in every scenario means an increase in memory foot print which could be a significant factor for targets which just want unwind information for debug

Given the similarities between eh_frame and debug_frame - could you explain the architectural problems with changing whatever code causes eh_frame to be generated to instead cause debug_frame to be generated, without the need for implementing a new streamer?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778