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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

In D78778#2444445 <https://reviews.llvm.org/D78778#2444445>, @dblaikie wrote:

> (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)

I will let Ram comment on this, we have collaborated on the patch and he is more familiar with the interactions of these flags.

>> 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?

I can comment on the second question. I started by just making changes to `DwarfCFIException`/`DwarfCFIExceptionBase`/`EHStreamer` (these are all streamers in the same hierarchy, from most specialized to least), and also considered inserting a new class in the hierarchy "above" `EHStreamer` where non-EH-specific unwind could be implemented. The result was just less clear, and there was no obvious way I could see to share any meaningful amount of code. The patch as-is does introduce a little more boilerplate, but at the time of writing I considered it a better tradeoff. I'm of course open to criticism and suggestions!

The other architectural issue is that `EHStreamer` is abstract, meaning the naive approach of having the new concrete `UnwindStreamer` above it in the hierarchy would imply deriving an abstract class from a concrete class, and would mean we would need to re-`delete` the methods implemented in `UnwindStreamer`, like `beginFunction`. Alternatively we would need a common abstract base for both `UnwindStreamer` and `EHStreamer`, but I determined that it would essentially be empty, at which point it is nearly equivalent to `AsmPrinterHandler`, and we get the code in the current patch.

Again, I'm not entirely happy with the apparent duplication, but a small amount of duplication seemed preferable to adding more conditions to the concrete implementation of `EHStreamer` or added complication in the type hierarchy which doesn't actually lead to non-trivial code sharing. YMMV and if one of these other approaches, or one that I overlooked, seems more attractive I'm happy to make the changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778