[PATCH] D78778: Add SupportsDebugUnwindInformation to MCAsmInfo

Tue May 12 11:49:19 PDT 2020

scott.linder added inline comments.

================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:359
+  /// Defaults to false.
+  bool SupportsDebugUnwindInformation = false;
+
----------------
echristo wrote:
> Can you enumerate, perhaps with some examples, when you expect this to be used?
The dependent revisions in the stack are the primary example. AMDGPU does not support exceptions, and lying and saying we do in AMDGPU's MCAsmInfo just to enable unwinding is not attractive/feasible as it affects codegen in other ways. We just want to enable generating unwind information to be consumed by a debugger. If there is a mechanism for this in LLVM which I missed then we should use that, or if there is a more straightforward way to tease out the two orthogonal concepts (EH and unwind information) without perturbing every other target then this should be scrapped for that approach.

I'm not particularly happy with the approach in this patch, but I think it is a pragmatic one with pretty straightforward semantics. Every other approach I considered seems to be either underhanded or require a relatively large-scale rewrite of all the existing EH support, which I am not prepared to take on right now.

================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:328

+  if (MMI->hasDebugInfo() &&
+      MAI->getExceptionHandlingType() == ExceptionHandling::None &&
----------------
RamNalamothu wrote:
> This ignores //ForceDwarfFrameSection// which is independent of //MMI->hasDebugInfo()//.
`ForceDwarfFrameSection` has no effect when `MAI->getExceptionHandlingType() == ExceptionHandling::None` though, so I believe this condition implicity respects it? The semantics of `MAI->doesSupportDebugUnwindInformation()` subsume `ForceDwarfFrameSection` anyway as it always produces `.debug_frame`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778