[PATCH] D78778: [AsmPrinter] Fix emitting CFI for debug when exceptions are not supported

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:378
 
+  /// Currently emitting CFI unwind information is entangled with supporting
+  /// the exceptions. When ExceptionsType == ExceptionHandling::None, this
----------------
RamNalamothu wrote:
> MaskRay wrote:
> > "Currently emitting CFI unwind information is entangled with supporting ..." This is a remark. It has minor state compared with the main comment. It can be added at the end or omitted.
> Doesn't my original sequence of statements clearly state why and for what `usesCFIForDebug()` is needed given the current state of things on if/when a .eh_frame or .debug_frame is emitted?
OK, this refers to `MCAsmInfo::ExceptionsType`.

I was confused.

Maybe this can be rephrased a bit, like 

Used by `MCAsmInfo::ExceptionsType == ExceptionHandling::None` targets to emit .debug_frame for debugging purposes.
Currently emitting CFI unwind information is entangled with supporting exceptions.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:351
   switch (MAI->getExceptionHandlingType()) {
+  // We may want to emit CFI for debug or when `ForceDwarfFrameSection`.
+  case ExceptionHandling::None:
----------------
RamNalamothu wrote:
> MaskRay wrote:
> > This comment is not clear. Does it only apply to `None`?
> Yes, will change it to
> 
> ```
> case ExceptionHandling::None:
>     // We may want to emit CFI for debug or when `ForceDwarfFrameSection`.
>     LLVM_FALLTHROUGH;
> case ExceptionHandling::SjLj:
> ```
Adding a blank line below `None`  works as well.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1067
+bool AsmPrinter::usesCFIForDebug() const {
+  return MAI->getExceptionHandlingType() == ExceptionHandling::None &&
+         ((MMI->hasDebugInfo() && ModuleCFISection == CFISection::Debug) ||
----------------
RamNalamothu wrote:
> MaskRay wrote:
> > Does AMDGPU use `MAI->getExceptionHandlingType() == ExceptionHandling::None` for .debug_frame?
> > 
> > Why can't it use `DwarfCFI`?
> > Does AMDGPU use `MAI->getExceptionHandlingType() == ExceptionHandling::None` for .debug_frame?
> 
> Yes and AMDGPU can't afford to have an `.eh_frame`, which is an `ALLOC` section, for debug purpose.
> 
> > Why can't it use `DwarfCFI`?
> 
> As mentioned earlier, AMDGPU don't support exceptions.
> 
> 
Ah, ok, now I understand this part.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778