[PATCH] D78778: Add SupportsDebugUnwindInformation to MCAsmInfo

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:328
 
+  if (MMI->hasDebugInfo() &&
+      MAI->getExceptionHandlingType() == ExceptionHandling::None &&
----------------
scott.linder wrote:
> RamNalamothu wrote:
> > RamNalamothu wrote:
> > > scott.linder wrote:
> > > > 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`.
> > > `ForceDwarfFrameSection` is independent otherwise there is the point in saying force something and this reflects in `MachineFunction::needsFrameMoves()`.
> > > 
> > > Yes, for AMDGPU we do get `.debug_frame` always, but this piece of code is generic and not specific to AMDGPU.
> > > Yes, for AMDGPU we do get .debug_frame always, but this piece of code is generic and not specific to AMDGPU.
> > 
> > 
> > I meant to say all other targets will be forced to return `true` from `MAI->doesSupportDebugUnwindInformation()` to get similar behavior like AMDGPU.
> I am still not sure I understand exactly what you mean, but it seems like you are saying we should change `ForceDwarfFrameSection` here to also have an effect when `MAI->getExceptionHandlingType() == ExceptionHandling::None`? In effect change this condition to:
> 
> ```
> if (MAI->getExceptionHandlingType() == ExceptionHandling::None && (ForceDwarfFrameSection || (MMI->hasDebugInfo() && MAI->doesSupportDebugUnwindInformation()))) { ... }
> ```
> 
> That seems very reasonable to me, assuming the intention of `ForceDwarfFrameSection` was to have an effect with EH disabled and the current state of things just represents a bug.
Yes, this condition check should be inline with `MachineFunction::needsFrameMoves()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78778