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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> 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)
>
> The fact that the unwind information could be generated for debug or exception handling use case is correctly modeled in //`MachineFunction::needsFrameMoves()`// and //`AsmPrinter::needsCFIMoves()`//, and forcing to generate //`.debug_frame`// is honored as expected here, but the issue is arising as the unwind information generation is implemented in //`class DwarfCFIException`// through the //`class EHStreamer`// base class

So there are two different codepaths that generate unwind information - presumably under different conditions? What are those different conditions? (when is the MachineFunction::needsFrameMoves path used, and when is the DwarfCFIException path used)

> ...
> Yeah, a test case could be added to demonstrate that this patch fix the AsmPrinter behavior to get //`.debug_frame`// generated for //`--force-dwarf-frame-section`// even when //`-fno-exceptions`// is specified.

Would be good to get that test case added.

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

Sorry, I wasn't really able to follow that description. I'll try to clarify/rephrase my question which might help:

Under some conditions, eh_frame is generated even under -fno-exceptions, right? And you'd like debug_frame to be generated instead/as well, in that case? I'm trying to understand why the generation of these two things (eh_frame/debug_frame) isn't close enough together as to make the fix for this issue be a single conditional somewhere, that picks whether which name to use (& some associated properties, like whether it's ALLOC).

Could you explain the codepath that generates eh_frame under basically the conditions you would like, and why that codepath can't handle being modified to generate debug_frame instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778