[MCAsmInfo] Support UsesCFIForDebug for targets with no exception handling

[RamNalamothu]

Summary

This change enables emitting CFI unwind information for debugging purpose
for targets with MCAsmInfo::ExceptionsType == ExceptionHandling::None.

Currently generating CFI unwind information is entangled with supporting
the exceptions, even when AsmPrinter explicitly recognizes that the unwind
tables are being generated as debug information.

In fact, the unwind information is not generated even if we specify
--force-dwarf-frame-section, unless exceptions are enabled. The LIT test
llvm/test/CodeGen/AMDGPU/debug_frame.ll demonstrates this behavior.

Enable this option for AMDGPU to prepare for future patches which add
complete CFI support.

[RamNalamothu]

@scott.linder is this not yet ready for review, as you have not added any reviewers?

[scott.linder]

In D78778#2029334, @RamNalamothu wrote:

@scott.linder is this not yet ready for review, as you have not added any reviewers?

No, it is ready to review, I just neglected to add any reviewers. I still haven't figured out how to manage a set of patches in a sane way on Phabricator, so I end up doing a lot manually.

[RamNalamothu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

This ignores ForceDwarfFrameSection which is independent of MMI->hasDebugInfo().

[echristo]

llvm/include/llvm/MC/MCAsmInfo.h

377

Can you enumerate, perhaps with some examples, when you expect this to be used?

[scott.linder]

Rebase

llvm/include/llvm/MC/MCAsmInfo.h

377

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.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

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.

[RamNalamothu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

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.

[RamNalamothu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

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.

[scott.linder]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

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.

[RamNalamothu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

Yes, this condition check should be inline with MachineFunction::needsFrameMoves().

[RamNalamothu]

Rebase and address feedback

[RamNalamothu]

Rebase

[RamNalamothu]

Rebase onto LLVM master

[cameron.mcinally]

Cray/HPE is motivated to see this patch land. My confidence is low in this area, but I don't see any review comments that are fatal. Any reason we can't move forward with this change?

[probinson]

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.

[RamNalamothu]

In 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 i.e. a .debug_frame is not generated
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
3. Ideally -fasynchronous-unwind-tables may want to generate .debug_frame instead of a .eh_frame. Some relevant discussion is here on lldb-dev list

Also, there was a long discussion, here and here, on separating the unwind support from exceptions handling logic in the past as well and some how that discussion did not end up with a patch.

[probinson]

I was not aware that .eh_frame needed to be an ALLOC section, although it makes sense now that you say it; thanks for that!

I don't feel qualified to approve this, but any reviewer will tell you: Needs a test.

[greened]

This is pretty far afield from my areas of expertise so I can't really comment on the overall approach of this patch. From a purely mechanical/style point of view this seems fine to me.

[scott.linder]

Rebase and ping

[scott.linder]

In D78778#2301702, @probinson wrote:

I was not aware that .eh_frame needed to be an ALLOC section, although it makes sense now that you say it; thanks for that!

I don't feel qualified to approve this, but any reviewer will tell you: Needs a test.

I can't seem to find another example of a test of an MCAsmInfo option, at least not without a target which sets it. I suppose a UnitTest which derives a new class from MCAsmInfo could work, but then I wonder why this hasn't been needed for any of the other options.

The tests in https://reviews.llvm.org/D76879 do exercise this, and it seemed like it would be easier to review as two separate commits. The current phabricator "stack" doesn't express the dependencies perfect here, I can move it around some to make it more clear.

[RamNalamothu]

Rebase onto LLVM Master

[dblaikie]

In D78778#2322163, @scott.linder wrote:

In D78778#2301702, @probinson wrote:

I was not aware that .eh_frame needed to be an ALLOC section, although it makes sense now that you say it; thanks for that!

I don't feel qualified to approve this, but any reviewer will tell you: Needs a test.

I can't seem to find another example of a test of an MCAsmInfo option, at least not without a target which sets it.

Yep, generally LLVM adds use of functionality available from at least one of the tools (llc, etc) in the patch that adds the functionality and tests it through that.

I suppose a UnitTest which derives a new class from MCAsmInfo could work, but then I wonder why this hasn't been needed for any of the other options.

The tests in https://reviews.llvm.org/D76879 do exercise this, and it seemed like it would be easier to review as two separate commits. The current phabricator "stack" doesn't express the dependencies perfect here, I can move it around some to make it more clear.

Generally adding code that's untested in the patch it's added in is unfavorable. Looks like this should be merged with D76879 - specifically because the tests should be assessed in the context of the new code and the new code should be assessed in the context of how the tests cover that functionality to ensure good coverage. (if there's a lot of patches between these two patches/bunch of other functionality - then maybe adding a flag to support optionally using this feature, rather than hardcoding/making it permanently on - then the code can be tested without turning on a half-implemented feature, and adding functionality with associated test coverage incrementally, then at the end removing the optional flag support and making it permanent/constantly enabled)

[scott.linder]

Merge with review to enable CFI for AMDGPU. This makes the commit
testable without needing to add a temporary flag to force the behavior.

[scott.linder]

In D78778#2356959, @dblaikie wrote:

In D78778#2322163, @scott.linder wrote:

In D78778#2301702, @probinson wrote:

I was not aware that .eh_frame needed to be an ALLOC section, although it makes sense now that you say it; thanks for that!

I don't feel qualified to approve this, but any reviewer will tell you: Needs a test.

I can't seem to find another example of a test of an MCAsmInfo option, at least not without a target which sets it.

Yep, generally LLVM adds use of functionality available from at least one of the tools (llc, etc) in the patch that adds the functionality and tests it through that.

I suppose a UnitTest which derives a new class from MCAsmInfo could work, but then I wonder why this hasn't been needed for any of the other options.

The tests in https://reviews.llvm.org/D76879 do exercise this, and it seemed like it would be easier to review as two separate commits. The current phabricator "stack" doesn't express the dependencies perfect here, I can move it around some to make it more clear.

Generally adding code that's untested in the patch it's added in is unfavorable. Looks like this should be merged with D76879 - specifically because the tests should be assessed in the context of the new code and the new code should be assessed in the context of how the tests cover that functionality to ensure good coverage. (if there's a lot of patches between these two patches/bunch of other functionality - then maybe adding a flag to support optionally using this feature, rather than hardcoding/making it permanently on - then the code can be tested without turning on a half-implemented feature, and adding functionality with associated test coverage incrementally, then at the end removing the optional flag support and making it permanent/constantly enabled)

Thank you for the feedback, I didn't think carefully about what would be testable at each point when I laid out how to structure the patch series. I took your advice and merged the two commits that must be present to test the new MCAsmInfo flag.

I need to go through the rest of the series and see if similar changes are needed, but I think this patch is ready for review again.

[scott.linder]

Fix bug where doesSupportDebugUnwindInformation precluded support for cfguard and (in the latest upstream) skipped the call to DebugHandlerBase::beginModule() for all registered handlers.

[scott.linder]

Rebase over changes which eliminate the spurious newlines in the assembly
output of the DebugInfo test.

Please take a look, or if anyone knows someone who is comfortable with this
code please add them as a reviewer :)

[dblaikie]

(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, @RamNalamothu wrote:

In 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 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 address? that are distinct from this case that it misses)

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?

[scott.linder]

In 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, @RamNalamothu wrote:

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

[RamNalamothu]

In 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, @RamNalamothu wrote:

In 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 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 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 and this streamer is added to AsmPrinter debug handlers only when exceptions are enabled and all the scenarios in MachineFunction::needsFrameMoves()/AsmPrinter::needsCFIMoves() are not considered here to decide when/what unwind information tables is to be generated. This patch considers all those scenarios while generating unwind tables (please have a look at llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:339 of this patch).

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.

[dblaikie]

In D78778#2446116, @RamNalamothu wrote:

In 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, @RamNalamothu wrote:

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

[scott.linder]

Delete UnwindStreamer, merging the behavior into DwarfCFIException. Also rename
the MCAsmInfo property to SupportsDebugOnlyCFI to be more direct/accurate about
the semantics.

[scott.linder]

In D78778#2447314, @dblaikie wrote:

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?

No worries, I think I got a little bit ahead of myself :^) I summarized the whole answer to the question you actually asked with " I started by just making changes to DwarfCFIException/DwarfCFIExceptionBase/EHStreamer..."

I started writing up a response describing the code path for the unwind emission and decided it would be easier to just draft a version of the commit without the separate streamer. Part of what I was trying to describe in earlier comments was that I had originally done this, but found that it only made already-complicated code more complicated. In implementing it again I don't know what specifically I didn't like about it before, though.

Let me know what you think of this iteration; I don't know how to localize it much more than this, as the DwarfCFI streamer needs to be created and we need to note that the CFI is for debug use only in AsmPrinter.cpp. The logical "one condition" that needs to change is shouldEmitCFI in DwarfCFIException.cpp.

[MaskRay]

Could you just add ExceptionsType = ExceptionHandling::DwarfCFI to AMDGPUMCAsmInfo.cpp? You can still make Asm->needsOnlyDebugCFIMoves so that .debug_frame will be generated while .eh_frame is not.

I do feel that we have a bunch of states which should be available to represent various needs (.eh_frame only, .debug_frame only, both, none) and support various options (-fasynchronous-unwind-tables), just probably that the states are not well organized...

Add SupportsDebugOnlyCFI as a workaround for targets which do not have EH support but which do support unwind information for debugging.

Perhaps just expand the abbreviations: "... do not support .eh_frame but support .debug_frame for debugging."

[MaskRay]

I think the thing we cannot get past is: ExceptionHandling::DwarfCFI is called exception handling and thus we expect it to be only useful with C++ (or other language) exceptions. However, the practice is that we overload it for non-exception .eh_frame/.ARM_extab/.ARM_exidx generation as well. So, adding a behavior to ExceptionHandling::None and introducing SupportsDebugUnwindInformation just seem to bring more complexity to me.

If ExceptionHandling were called something else, this might be less misleading.

[scott.linder]

In D78778#2449310, @MaskRay wrote:

Could you just add ExceptionsType = ExceptionHandling::DwarfCFI to AMDGPUMCAsmInfo.cpp? You can still make Asm->needsOnlyDebugCFIMoves so that .debug_frame will be generated while .eh_frame is not.

I do feel that we have a bunch of states which should be available to represent various needs (.eh_frame only, .debug_frame only, both, none) and support various options (-fasynchronous-unwind-tables), just probably that the states are not well organized...

This was our first approach, but we found other places which were sensitive to ExceptionsType being set. Maybe those aren't present anymore; is anyone aware if that option should affect CodeGen? I can take another look now, and see if I can point to the issues we had.

The more fundamental thing, which is another way of framing what you say about organization, is that unwinding should be decoupled from exception handling. This seems to have been a pain point for at least ARM (Ram mentioned http://lists.llvm.org/pipermail/cfe-dev/2014-February/035154.html earlier, which comes to the same conclusion).

Add SupportsDebugOnlyCFI as a workaround for targets which do not have EH support but which do support unwind information for debugging.

Perhaps just expand the abbreviations: "... do not support .eh_frame but support .debug_frame for debugging."

Will do, begin explicit about the limit scope of the option seems good.

[scott.linder]

In D78778#2449342, @MaskRay wrote:

I think the thing we cannot get past is: ExceptionHandling::DwarfCFI is called exception handling and thus we expect it to be only useful with C++ (or other language) exceptions. However, the practice is that we overload it for non-exception .eh_frame/.ARM_extab/.ARM_exidx generation as well. So, adding a behavior to ExceptionHandling::None and introducing SupportsDebugUnwindInformation just seem to bring more complexity to me.

If ExceptionHandling were called something else, this might be less misleading.

Right, I am open to suggestions, but it also just mirrors the implementation in that the code for doing the debug-only, no-exceptions unwind information is still contained in EHStreamer/DwarfCFIException. Those could also be renamed, but then most of the code in them is irrelevant to/complicates the debug case.

That's how I ended up at "make a new streamer that is honest about being for debug-only DWARF CFI", and it happened to be nearly empty, so I didn't even have it share code with the exception variants. Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?

[MaskRay]

In D78778#2449379, @scott.linder wrote:

In D78778#2449342, @MaskRay wrote:

I think the thing we cannot get past is: ExceptionHandling::DwarfCFI is called exception handling and thus we expect it to be only useful with C++ (or other language) exceptions. However, the practice is that we overload it for non-exception .eh_frame/.ARM_extab/.ARM_exidx generation as well. So, adding a behavior to ExceptionHandling::None and introducing SupportsDebugUnwindInformation just seem to bring more complexity to me.

If ExceptionHandling were called something else, this might be less misleading.

Right, I am open to suggestions, but it also just mirrors the implementation in that the code for doing the debug-only, no-exceptions unwind information is still contained in EHStreamer/DwarfCFIException. Those could also be renamed, but then most of the code in them is irrelevant to/complicates the debug case.

That's how I ended up at "make a new streamer that is honest about being for debug-only DWARF CFI", and it happened to be nearly empty, so I didn't even have it share code with the exception variants. Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?

Yes, some renaming can help. I believe MCAsmInfo has sufficient states to encode all the needs.

Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?

Having both UnwindTable and ExceptionHandling enums sounds good. isCFIMoveForDebugging (the name confused me quite a bit) probably should be folded into the new representations.

Maybe those aren't present anymore; is anyone aware if that option should affect CodeGen?

I have scanned the call sites of getExceptionHandlingType. DwarfEHPrepapre.cpp can replace resume with _Unwind_Resume. However, if you don't use C++ exceptions, the pass should be a no-op.
There should be no target-independent pass which can affect codegen (wasm and arm are complex and I cannot ensure they don't affect codegen but those are their target decisions)

[scott.linder]

In D78778#2449409, @MaskRay wrote:

In D78778#2449379, @scott.linder wrote:

In D78778#2449342, @MaskRay wrote:

I think the thing we cannot get past is: ExceptionHandling::DwarfCFI is called exception handling and thus we expect it to be only useful with C++ (or other language) exceptions. However, the practice is that we overload it for non-exception .eh_frame/.ARM_extab/.ARM_exidx generation as well. So, adding a behavior to ExceptionHandling::None and introducing SupportsDebugUnwindInformation just seem to bring more complexity to me.

If ExceptionHandling were called something else, this might be less misleading.

Right, I am open to suggestions, but it also just mirrors the implementation in that the code for doing the debug-only, no-exceptions unwind information is still contained in EHStreamer/DwarfCFIException. Those could also be renamed, but then most of the code in them is irrelevant to/complicates the debug case.

That's how I ended up at "make a new streamer that is honest about being for debug-only DWARF CFI", and it happened to be nearly empty, so I didn't even have it share code with the exception variants. Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?

Yes, some renaming can help. I believe MCAsmInfo has sufficient states to encode all the needs.

Maybe instead of an ad-hoc option, though, it could be another enum DebugUnwindTables { None; DwarfCFI; }? And rename the EHStreamer hierarchy to refer to both exceptions and unwind, and effectively have the same code as in the current patch but with more descriptive names?

Having both UnwindTable and ExceptionHandling enums sounds good. isCFIMoveForDebugging (the name confused me quite a bit) probably should be folded into the new representations.

Ok, I can draft up something with the new enum. As for isCFIMoveForDebugging I agreed and deleted it in https://reviews.llvm.org/D76519, which I see you've already commented on, thank you!

Maybe those aren't present anymore; is anyone aware if that option should affect CodeGen?

I have scanned the call sites of getExceptionHandlingType. DwarfEHPrepapre.cpp can replace resume with _Unwind_Resume. However, if you don't use C++ exceptions, the pass should be a no-op.
There should be no target-independent pass which can affect codegen (wasm and arm are complex and I cannot ensure they don't affect codegen but those are their target decisions)

Taking a look again, the difference in codegen was due to TargetPassConfig::addPassesToHandleExceptions no longer adding the LowerInvoke pass, IIRC. I'm not actually sure we rely on this anywhere outside of some tests, but at that point I determined that we shouldn't "lie" about our ExceptionsType if it can affect codegen. Adding the separate, mostly-orthogonal enum for unwind information should solve this.

[dblaikie]

In D78778#2449224, @scott.linder wrote:

In D78778#2447314, @dblaikie wrote:

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?

No worries, I think I got a little bit ahead of myself :^) I summarized the whole answer to the question you actually asked with " I started by just making changes to DwarfCFIException/DwarfCFIExceptionBase/EHStreamer..."

I started writing up a response describing the code path for the unwind emission and decided it would be easier to just draft a version of the commit without the separate streamer. Part of what I was trying to describe in earlier comments was that I had originally done this, but found that it only made already-complicated code more complicated. In implementing it again I don't know what specifically I didn't like about it before, though.

Let me know what you think of this iteration; I don't know how to localize it much more than this, as the DwarfCFI streamer needs to be created and we need to note that the CFI is for debug use only in AsmPrinter.cpp. The logical "one condition" that needs to change is shouldEmitCFI in DwarfCFIException.cpp.

Thanks - that looks in the ballpark of what I'd expect. I'll leave it to you & @MaskRay to hash out the rest of the details.

[RamNalamothu]

How does it look now, with some renaming I did?

I have incorporated the changes from https://reviews.llvm.org/D76519 into update.

[RamNalamothu]

Rebase and ping

[RamNalamothu]

Rebase onto ToT.

[RamNalamothu]

@dblaikie, @MaskRay

I think I have addressed the feedback. Could you please take a look now?

By the way, this patch (and the associated patch series) is under review for more than 10 months now and would appreciate help in closing these sooner.

[RamNalamothu]

Rebase and ping

[dblaikie]

llvm/include/llvm/MC/MCAsmInfo.h

51–55

Sorry, paging this all back in. Why is this necessary? THis looks like the sort of change that would be necessary to allow /both/ eh_frame and debug_frame to be emitted at the same time. Is that what this is about? That doesn't seem to line up with my reading of the patch description, which seems to be about being able to emit debug_frame even when not using exceptions - but wouldn't require anything to say "I want eh_frame and debug_frame at the same time" I think?

52

377

at least the naming of this variable seems a bit off to me

Could you explain a bit more why you had to "lie" to get debug_frame in the past? Which other systems exist/does LLVM support where SupportsDebugOnlyCFI would be problematic/wrong (if they opted out of exceptions/don't support exceptions)? Or could we just assume everyone's actually fine with debug_frame even if they don't have exceptions enabled?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

355

Minor thing, I think this can be written more simply

[RamNalamothu]

Remove the changes that were committed seperately in https://reviews.llvm.org/D76519.

[RamNalamothu]

@dblaikie wrote:
Sorry, paging this all back in. Why is this necessary? THis looks like the sort of change that would be necessary to allow /both/ eh_frame and debug_frame to be emitted at the same time. Is that what this is about? That doesn't seem to line up with my reading of the patch description, which seems to be about being able to emit debug_frame even when not using exceptions - but wouldn't require anything to say "I want eh_frame and debug_frame at the same time" I think?

I thought making the https://reviews.llvm.org/D76519 changes in this review gives better overall context but it looks like that was adding more confusion. So, I stripped those changes from this review.

Does the patch makes sense now, in terms of what it intends to achieve i.e. emit CFI into .debug_frame for debug when exceptions are not supported?

[dblaikie]

Thanks - as much as I'm following this, it seems OK to me.

[RamNalamothu]

Fix couple of WebAsembly debug info LIT tests, as we now emit .debug_frame when no exceptions.

[dblaikie]

In D78778#2719704, @RamNalamothu wrote:

Fix couple of WebAsembly debug info LIT tests, as we now emit .debug_frame when no exceptions.

Hmm - was this intended to change the behavior of WebAssembly? Could you get someone with some WebAssembly context to double check this is desirable behavior?

[MaskRay]

This comment has been deleted.

llvm/include/llvm/CodeGen/AsmPrinter.h

378 ↗

(On Diff #340842)

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

379 ↗

(On Diff #340842)

ExceptionsType is not mentioned.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

This comment is not clear. Does it only apply to None?

1012

Does AMDGPU use MAI->getExceptionHandlingType() == ExceptionHandling::None for .debug_frame?

Why can't it use DwarfCFI?

[RamNalamothu]

llvm/include/llvm/CodeGen/AsmPrinter.h

378 ↗

(On Diff #340842)

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?

379 ↗

(On Diff #340842)

ExceptionsType is not mentioned.

Sorry, I didn't understand that.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

Yes, will change it to

case ExceptionHandling::None:
    // We may want to emit CFI for debug or when `ForceDwarfFrameSection`.
    LLVM_FALLTHROUGH;
case ExceptionHandling::SjLj:

1012

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.

[MaskRay]

llvm/include/llvm/CodeGen/AsmPrinter.h

378 ↗

(On Diff #340842)

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.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

327

Adding a blank line below None works as well.

1012

Ah, ok, now I understand this part.

[MaskRay]

I was confused by the subject (this is new support, not fixing a bug). The updated wasm tests contradict "while maintaining the status quo on how the other things were."
Also, I think the summary may be easier to understand if you just state in the beginning that this allows ExceptionHandling::None to use .debug_frame.
You can then explain some concepts, if needed.

[MaskRay]

llvm/test/MC/WebAssembly/debug-info.ll

172 ↗

(On Diff #340842)

+ @sbc100 for wasm .debug_frame change.

[RamNalamothu]

In D78778#2720397, @dblaikie wrote:

In D78778#2719704, @RamNalamothu wrote:

Fix couple of WebAsembly debug info LIT tests, as we now emit .debug_frame when no exceptions.

Hmm - was this intended to change the behavior of WebAssembly? Could you get someone with some WebAssembly context to double check this is desirable behavior?

Not intended but probably the LIT test updates are expected for WebAssembly. Adding @aheejin as well to review those.
Please note that those LIT tests are not explicitly checking for either presence/non-presense of .eh_frame or .debug_frame and might have just tried to match the compiler output back then.

However, while I still feel the current usesCFIForDebug() approach is in the right direction, please let me know if the preferred approach is a target configurable option and if yes, I can update this review with https://reviews.llvm.org/D101592 changes (not meant to be reviewed there).

[aheejin]

In D78778#2727655, @RamNalamothu wrote:

In D78778#2720397, @dblaikie wrote:

In D78778#2719704, @RamNalamothu wrote:

Fix couple of WebAsembly debug info LIT tests, as we now emit .debug_frame when no exceptions.

Hmm - was this intended to change the behavior of WebAssembly? Could you get someone with some WebAssembly context to double check this is desirable behavior?

Not intended but probably the LIT test updates are expected for WebAssembly. Adding @aheejin as well to review those.
Please note that those LIT tests are not explicitly checking for either presence/non-presense of .eh_frame or .debug_frame and might have just tried to match the compiler output back then.

However, while I still feel the current usesCFIForDebug() approach is in the right direction, please let me know if the preferred approach is a target configurable option and if yes, I can update this review with https://reviews.llvm.org/D101592 changes (not meant to be reviewed there).

I am not very familiar with these tests. Perhaps any of @aardappel @sbc100 @yurydelendik would know.

[RamNalamothu]

In D78778#2729953, @aheejin wrote:

In D78778#2727655, @RamNalamothu wrote:

In D78778#2720397, @dblaikie wrote:

In D78778#2719704, @RamNalamothu wrote:

Fix couple of WebAsembly debug info LIT tests, as we now emit .debug_frame when no exceptions.

Hmm - was this intended to change the behavior of WebAssembly? Could you get someone with some WebAssembly context to double check this is desirable behavior?

Not intended but probably the LIT test updates are expected for WebAssembly. Adding @aheejin as well to review those.
Please note that those LIT tests are not explicitly checking for either presence/non-presense of .eh_frame or .debug_frame and might have just tried to match the compiler output back then.

However, while I still feel the current usesCFIForDebug() approach is in the right direction, please let me know if the preferred approach is a target configurable option and if yes, I can update this review with https://reviews.llvm.org/D101592 changes (not meant to be reviewed there).

I am not very familiar with these tests. Perhaps any of @aardappel @sbc100 @yurydelendik would know.

I went ahead and updated the diff with the changes from https://reviews.llvm.org/D101592 i.e. make the UsesCFIForDebug option target configurable so that this review can be closed sooner and enable making progress on the rest of the patches in the series.
And later if all targets want to emit .debug_frame, by default, when exceptions are not supported, things can be changed accordingly then.

[RamNalamothu]

Does it look good now?

[RamNalamothu]

Fix llvm/test/CodeGen/AMDGPU/ptr-arg-dbg-value.ll failure on Windows.

[MaskRay]

llvm/test/CodeGen/AMDGPU/debug_frame.ll

1 ↗

(On Diff #343095)

It should be sufficient testing the default (-filetype=asm) and avoiding llvm-readelf -S.

You can then test .cfi_sections .debug_frame.

llvm/test/CodeGen/AMDGPU/split-arg-dbg-value.ll

147

Will be good aligning the directive with other symbol labels.

[RamNalamothu]

Address feedback on LIT tests.

[RamNalamothu]

If there are no further comments, I will commit this once the build is completed.

Thank you.

[MaskRay]

LGTM, with one last nit

llvm/test/CodeGen/AMDGPU/debug_frame.ll

8 ↗

(On Diff #343112)

You can share the same check prefix since they are the same.

[RamNalamothu]

Update llvm/test/CodeGen/AMDGPU/debug_frame.ll test.

llvm/include/llvm/MC/MCAsmInfo.h

enum class EncodingType {
   MIPS = Alpha,
 };
 
 } // end namespace WinEH
 
 namespace LCOMM {
 
 enum LCOMMType { NoAlignment, ByteAlignment, Log2Alignment };
 
 } // end namespace LCOMM
 
 /// This class is intended to be used as a base class for asm
 /// properties and features specific to the target.
 class MCAsmInfo {
 public:
   /// Assembly character literal syntax types.
   enum AsmCharLiteralSyntax {
     ACLS_Unknown, /// Unknown; character literals not used by LLVM for this
                   /// target.
     ACLS_SingleQuotePrefix, /// The desired character is prefixed by a single
                             /// quote, e.g., `'A`.
protected:
   MCSymbolAttr ProtectedVisibilityAttr = MCSA_Protected;
 
   //===--- Dwarf Emission Directives -----------------------------------===//
 
   /// True if target supports emission of debugging information.  Defaults to
   /// false.
   bool SupportsDebugInformation = false;
 
+  /// True if target supports emitting .debug_frame unwind information when
+  /// ExceptionsType = ExceptionHandling::None and debug info is requested.
+  /// Defaults to false.
+  bool SupportsDebugUnwindInformation = false;
+
   /// Exception handling format for the target.  Defaults to None.
   ExceptionHandling ExceptionsType = ExceptionHandling::None;
 
   /// Windows exception handling data (.pdata) encoding.  Defaults to Invalid.
   WinEH::EncodingType WinEHEncodingType = WinEH::EncodingType::Invalid;
 
   /// True if Dwarf2 output generally uses relocations for references to other
   /// .debug_* sections.
public:
   }
 
   MCSymbolAttr getProtectedVisibilityAttr() const {
     return ProtectedVisibilityAttr;
   }
 
   bool doesSupportDebugInformation() const { return SupportsDebugInformation; }
 
+  bool doesSupportDebugUnwindInformation() const {
+    return SupportsDebugUnwindInformation;
+  }
+
   bool doesSupportExceptionHandling() const {
     return ExceptionsType != ExceptionHandling::None;
   }
 
   ExceptionHandling getExceptionHandlingType() const { return ExceptionsType; }
   WinEH::EncodingType getWinEHEncodingType() const { return WinEHEncodingType; }
 
   void setExceptionsType(ExceptionHandling EH) {

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

 //===- AsmPrinter.cpp - Common AsmPrinter code ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the AsmPrinter class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/AsmPrinter.h"
 #include "CodeViewDebug.h"
 #include "DwarfDebug.h"
 #include "DwarfException.h"
+#include "UnwindStreamer.h"
 #include "WasmException.h"
 #include "WinCFGuard.h"
 #include "WinException.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"

 using namespace llvm;
 
 #define DEBUG_TYPE "asm-printer"
 
 static const char *const DWARFGroupName = "dwarf";
 static const char *const DWARFGroupDescription = "DWARF Emission";
 static const char *const DbgTimerName = "emit";
 static const char *const DbgTimerDescription = "Debug Info Emission";
+static const char *const UnwindTimerName = "write_unwind";
+static const char *const UnwindTimerDescription = "DWARF Unwind Writer";
 static const char *const EHTimerName = "write_exception";
 static const char *const EHTimerDescription = "DWARF Exception Writer";
 static const char *const CFGuardName = "Control Flow Guard";
 static const char *const CFGuardDescription = "Control Flow Guard";
 static const char *const CodeViewLineTablesGroupName = "linetables";
 static const char *const CodeViewLineTablesGroupDescription =
   "CodeView Line Tables";
 
if (MAI->doesSupportDebugInformation()) {
     if (!EmitCodeView || M.getDwarfVersion()) {
       DD = new DwarfDebug(this);
       Handlers.emplace_back(std::unique_ptr<DwarfDebug>(DD), DbgTimerName,
                             DbgTimerDescription, DWARFGroupName,
                             DWARFGroupDescription);
     }
   }
 
+  if (MAI->getExceptionHandlingType() == ExceptionHandling::None &&
+      (TM.Options.ForceDwarfFrameSection ||
+       (MMI->hasDebugInfo() && MAI->doesSupportDebugUnwindInformation()))) {
+    isCFIMoveForDebugging = true;
+    Handlers.emplace_back(std::make_unique<UnwindStreamer>(this),
+                          UnwindTimerName, UnwindTimerDescription,
+                          DWARFGroupName, DWARFGroupDescription);
+  } else {
     switch (MAI->getExceptionHandlingType()) {
     case ExceptionHandling::SjLj:
     case ExceptionHandling::DwarfCFI:
     case ExceptionHandling::ARM:
       isCFIMoveForDebugging = true;
       if (MAI->getExceptionHandlingType() != ExceptionHandling::DwarfCFI)
         break;
-    for (auto &F: M.getFunctionList()) {
+      for (auto &F : M.getFunctionList()) {
         // If the module contains any function with unwind data,
         // .eh_frame has to be emitted.
         // Ignore functions that won't get emitted.
         if (!F.isDeclarationForLinker() && F.needsUnwindTableEntry()) {
           isCFIMoveForDebugging = false;
           break;
         }
       }
       break;
     default:
       isCFIMoveForDebugging = false;
       break;
     }
 
     EHStreamer *ES = nullptr;
     switch (MAI->getExceptionHandlingType()) {
     case ExceptionHandling::None:
       break;
     case ExceptionHandling::SjLj:
     case ExceptionHandling::DwarfCFI:
       ES = new DwarfCFIException(this);
       break;
     case ExceptionHandling::ARM:
       ES = new ARMException(this);
       break;
     case ExceptionHandling::WinEH:
       switch (MAI->getWinEHEncodingType()) {
-    default: llvm_unreachable("unsupported unwinding information encoding");
+      default:
+        llvm_unreachable("unsupported unwinding information encoding");
       case WinEH::EncodingType::Invalid:
         break;
       case WinEH::EncodingType::X86:
       case WinEH::EncodingType::Itanium:
         ES = new WinException(this);
         break;
       }
       break;
     case ExceptionHandling::Wasm:
       ES = new WasmException(this);
       break;
     }
     if (ES)
       Handlers.emplace_back(std::unique_ptr<EHStreamer>(ES), EHTimerName,
                             EHTimerDescription, DWARFGroupName,
                             DWARFGroupDescription);
+  }
 
   // Emit tables for any value of cfguard flag (i.e. cfguard=1 or cfguard=2).
   if (mdconst::extract_or_null<ConstantInt>(M.getModuleFlag("cfguard")))
     Handlers.emplace_back(std::make_unique<WinCFGuard>(this), CFGuardName,
                           CFGuardDescription, DWARFGroupName,
                           DWARFGroupDescription);
 
   for (const HandlerInfo &HI : Handlers) {
AsmPrinter::CFIMoveType AsmPrinter::needsCFIMoves() const {
   return CFI_M_None;
 }
 
 bool AsmPrinter::needsSEHMoves() {
   return MAI->usesWindowsCFI() && MF->getFunction().needsUnwindTableEntry();
 }
 
 void AsmPrinter::emitCFIInstruction(const MachineInstr &MI) {
   ExceptionHandling ExceptionHandlingType = MAI->getExceptionHandlingType();
-  if (ExceptionHandlingType != ExceptionHandling::DwarfCFI &&
+  if (!MAI->doesSupportDebugUnwindInformation() &&
+      ExceptionHandlingType != ExceptionHandling::DwarfCFI &&
       ExceptionHandlingType != ExceptionHandling::ARM)
     return;
 
   if (needsCFIMoves() == CFI_M_None)
     return;
 
   // If there is no "real" instruction following this CFI instruction, skip
   // emitting it; it would be beyond the end of the function's FDE range.

llvm/lib/CodeGen/AsmPrinter/CMakeLists.txt

add_llvm_component_library(LLVMAsmPrinter
   DwarfUnit.cpp
   EHStreamer.cpp
   ErlangGCPrinter.cpp
   OcamlGCPrinter.cpp
   WinCFGuard.cpp
   WinException.cpp
   CodeViewDebug.cpp
   WasmException.cpp
+  UnwindStreamer.cpp
 
   DEPENDS
   intrinsics_gen
   )

llvm/lib/CodeGen/AsmPrinter/UnwindStreamer.h

+//===- UnwindStreamer.h - Unwind Directive Streamer -------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains support for writing unwind info into assembly files.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_CODEGEN_ASMPRINTER_UNWINDSTREAMER_H
+#define LLVM_LIB_CODEGEN_ASMPRINTER_UNWINDSTREAMER_H
+
+#include "llvm/CodeGen/AsmPrinterHandler.h"
+#include "llvm/Support/Compiler.h"
+
+namespace llvm {
+
+class AsmPrinter;
+class MachineInstr;
+class MCSymbol;
+
+/// Emits unwind info directives.
+class LLVM_LIBRARY_VISIBILITY UnwindStreamer : public AsmPrinterHandler {
+protected:
+  /// Target of directive emission.
+  AsmPrinter *Asm;
+
+  /// Per-module flag to indicate if .debug_frame has been emitted yet.
+  bool HasEmittedDebugFrame = false;
+
+public:
+  UnwindStreamer(AsmPrinter *A);
+  ~UnwindStreamer() override;
+
+  // Unused.
+  void setSymbolSize(const MCSymbol *Sym, uint64_t Size) override {}
+  void endModule() override {}
+  void beginInstruction(const MachineInstr *MI) override {}
+  void endInstruction() override {}
+
+  void beginFunction(const MachineFunction *MF) override;
+  void endFunction(const MachineFunction *MF) override;
+};
+
+} // end namespace llvm
+
+#endif // LLVM_LIB_CODEGEN_ASMPRINTER_UNWINDSTREAMER_H

llvm/lib/CodeGen/AsmPrinter/UnwindStreamer.cpp

+//===- CodeGen/AsmPrinter/UnwindStreamer.cpp - Unwind Directive Streamer --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains support for writing unwind info into assembly files.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnwindStreamer.h"
+#include "llvm/CodeGen/AsmPrinter.h"
+#include "llvm/MC/MCStreamer.h"
+
+using namespace llvm;
+
+namespace llvm {
+class MachineFunction;
+} // end namespace llvm
+
+UnwindStreamer::UnwindStreamer(AsmPrinter *A) : Asm(A) {}
+
+UnwindStreamer::~UnwindStreamer() = default;
+
+void UnwindStreamer::beginFunction(const MachineFunction *MF) {
+  assert(Asm->needsCFIMoves() == AsmPrinter::CFI_M_Debug);
+  if (!HasEmittedDebugFrame) {
+    Asm->OutStreamer->emitCFISections(false, true);
+    HasEmittedDebugFrame = true;
+  }
+  Asm->OutStreamer->emitCFIStartProc(/*IsSimple=*/false);
+}
+
+void UnwindStreamer::endFunction(const MachineFunction *MF) {
+  Asm->OutStreamer->emitCFIEndProc();
+}

llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCAsmInfo.cpp

AMDGPUMCAsmInfo::AMDGPUMCAsmInfo(const Triple &TT,
 
   //===--- Global Variable Emission Directives --------------------------===//
   HasAggressiveSymbolFolding = true;
   COMMDirectiveAlignmentIsInBytes = false;
   HasNoDeadStrip = true;
   //===--- Dwarf Emission Directives -----------------------------------===//
   SupportsDebugInformation = true;
   DwarfRegNumForCFI = true;
+  SupportsDebugUnwindInformation = true;
 
   UseIntegratedAssembler = false;
 }
 
 bool AMDGPUMCAsmInfo::shouldOmitSectionDirective(StringRef SectionName) const {
   return SectionName == ".hsatext" || SectionName == ".hsadata_global_agent" ||
          SectionName == ".hsadata_global_program" ||
          SectionName == ".hsarodata_readonly_agent" ||

llvm/test/CodeGen/AMDGPU/split-arg-dbg-value.ll

 ; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx900 < %s | FileCheck -check-prefix=GCN %s
 ; Make sure dbg_value reports something for argument registers when they are split into multiple registers
 
 define hidden <4 x float> @split_v4f32_arg(<4 x float> returned %arg) local_unnamed_addr #0 !dbg !7 {
 ; GCN-LABEL: split_v4f32_arg:
 ; GCN:       .Lfunc_begin0:
 ; GCN-NEXT:    .file 0
 ; GCN-NEXT:    .loc 0 3 0 ; /tmp/dbg.cl:3:0
+; GCN-NEXT:    .cfi_sections .debug_frame
+; GCN-NEXT:    .cfi_startproc
 ; GCN-NEXT:  ; %bb.0:
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 96 32] $vgpr3
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 64 32] $vgpr2
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr1
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr0
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GCN-NEXT:  .Ltmp0:
 ; GCN-NEXT:    .loc 0 4 5 prologue_end ; /tmp/dbg.cl:4:5
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 ; GCN-NEXT:  .Ltmp1:
+; GCN:    .cfi_endproc
   call void @llvm.dbg.value(metadata <4 x float> %arg, metadata !18, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !19
   ret <4 x float> %arg, !dbg !20
 }
 
 define hidden <4 x float> @split_v4f32_multi_arg(<4 x float> %arg0, <2 x float> %arg1) local_unnamed_addr #0 !dbg !21 {
 ; GCN-LABEL: split_v4f32_multi_arg:
 ; GCN:       .Lfunc_begin1:
 ; GCN-NEXT:    .loc 0 7 0 ; /tmp/dbg.cl:7:0
+; GCN-NEXT:    .cfi_startproc
 ; GCN-NEXT:  ; %bb.0:
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_multi_arg:arg1 <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr5
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_multi_arg:arg1 <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr4
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_multi_arg:arg0 <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 96 32] $vgpr3
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_multi_arg:arg0 <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 64 32] $vgpr2
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_multi_arg:arg0 <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr1
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f32_multi_arg:arg0 <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr0
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GCN-NEXT:  .Ltmp2:
 ; GCN-NEXT:    .loc 0 8 17 prologue_end ; /tmp/dbg.cl:8:17
 ; GCN-NEXT:    v_add_f32_e32 v0, v4, v0
 ; GCN-NEXT:  .Ltmp3:
 ; GCN-NEXT:    v_add_f32_e32 v1, v5, v1
 ; GCN-NEXT:  .Ltmp4:
 ; GCN-NEXT:    v_add_f32_e32 v2, v4, v2
 ; GCN-NEXT:  .Ltmp5:
 ; GCN-NEXT:    v_add_f32_e32 v3, v5, v3
 ; GCN-NEXT:  .Ltmp6:
 ; GCN-NEXT:    .loc 0 8 5 is_stmt 0 ; /tmp/dbg.cl:8:5
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 ; GCN-NEXT:  .Ltmp7:
+; GCN:    .cfi_endproc
   call void @llvm.dbg.value(metadata <4 x float> %arg0, metadata !29, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !31
   call void @llvm.dbg.value(metadata <2 x float> %arg1, metadata !30, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !31
   %tmp = shufflevector <2 x float> %arg1, <2 x float> undef, <4 x i32> <i32 0, i32 1, i32 0, i32 1>, !dbg !32
   %add = fadd <4 x float> %tmp, %arg0, !dbg !33
   ret <4 x float> %add, !dbg !34
 }
 
 define hidden <4 x half> @split_v4f16_arg(<4 x half> returned %arg) local_unnamed_addr #0 !dbg !35 {
 ; GCN-LABEL: split_v4f16_arg:
 ; GCN:       .Lfunc_begin2:
 ; GCN-NEXT:    .loc 0 11 0 is_stmt 1 ; /tmp/dbg.cl:11:0
+; GCN-NEXT:    .cfi_startproc
 ; GCN-NEXT:  ; %bb.0:
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f16_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr1
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v4f16_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr0
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GCN-NEXT:  .Ltmp8:
 ; GCN-NEXT:    .loc 0 12 5 prologue_end ; /tmp/dbg.cl:12:5
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 ; GCN-NEXT:  .Ltmp9:
+; GCN:    .cfi_endproc
   call void @llvm.dbg.value(metadata <4 x half> %arg, metadata !42, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !43
   ret <4 x half> %arg, !dbg !44
 }
 
 define hidden double @split_f64_arg(double returned %arg) local_unnamed_addr #0 !dbg !45 {
 ; GCN-LABEL: split_f64_arg:
 ; GCN:       .Lfunc_begin3:
 ; GCN-NEXT:    .loc 0 15 0 ; /tmp/dbg.cl:15:0
+; GCN-NEXT:    .cfi_startproc
 ; GCN-NEXT:  ; %bb.0:
 ; GCN-NEXT:    ;DEBUG_VALUE: split_f64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr1
 ; GCN-NEXT:    ;DEBUG_VALUE: split_f64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr0
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GCN-NEXT:  .Ltmp10:
 ; GCN-NEXT:    .loc 0 16 5 prologue_end ; /tmp/dbg.cl:16:5
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 ; GCN-NEXT:  .Ltmp11:
+; GCN:    .cfi_endproc
   call void @llvm.dbg.value(metadata double %arg, metadata !50, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !51
   ret double %arg, !dbg !52
 }
 
 define hidden <2 x double> @split_v2f64_arg(<2 x double> returned %arg) local_unnamed_addr #0 !dbg !53 {
 ; GCN-LABEL: split_v2f64_arg:
 ; GCN:       .Lfunc_begin4:
 ; GCN-NEXT:    .loc 0 19 0 ; /tmp/dbg.cl:19:0
+; GCN-NEXT:    .cfi_startproc
 ; GCN-NEXT:  ; %bb.0:
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v2f64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 96 32] $vgpr3
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v2f64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 64 32] $vgpr2
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v2f64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr1
 ; GCN-NEXT:    ;DEBUG_VALUE: split_v2f64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr0
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GCN-NEXT:  .Ltmp12:
 ; GCN-NEXT:    .loc 0 20 5 prologue_end ; /tmp/dbg.cl:20:5
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 ; GCN-NEXT:  .Ltmp13:
+; GCN:    .cfi_endproc
   call void @llvm.dbg.value(metadata <2 x double> %arg, metadata !59, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !60
   ret <2 x double> %arg, !dbg !61
 }
 
 define hidden i64 @split_i64_arg(i64 returned %arg) local_unnamed_addr #0 !dbg !62 {
 ; GCN-LABEL: split_i64_arg:
 ; GCN:       .Lfunc_begin5:
 ; GCN-NEXT:    .loc 0 23 0 ; /tmp/dbg.cl:23:0
+; GCN-NEXT:    .cfi_startproc
 ; GCN-NEXT:  ; %bb.0:
 ; GCN-NEXT:    ;DEBUG_VALUE: split_i64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr1
 ; GCN-NEXT:    ;DEBUG_VALUE: split_i64_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr0
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GCN-NEXT:  .Ltmp14:
 ; GCN-NEXT:    .loc 0 24 5 prologue_end ; /tmp/dbg.cl:24:5
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 ; GCN-NEXT:  .Ltmp15:
+; GCN:    .cfi_endproc
   call void @llvm.dbg.value(metadata i64 %arg, metadata !67, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !68
   ret i64 %arg, !dbg !69
 }
 
 define hidden i8 addrspace(1)* @split_ptr_arg(i8 addrspace(1)* readnone returned %arg) local_unnamed_addr #0 !dbg !70 {
 ; GCN-LABEL: split_ptr_arg:
 ; GCN:       .Lfunc_begin6:
 ; GCN-NEXT:    .loc 0 27 0 ; /tmp/dbg.cl:27:0
+; GCN-NEXT:    .cfi_startproc
 ; GCN-NEXT:  ; %bb.0:
 ; GCN-NEXT:    ;DEBUG_VALUE: split_ptr_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 32 32] $vgpr1
 ; GCN-NEXT:    ;DEBUG_VALUE: split_ptr_arg:arg <- [DW_OP_constu 1, DW_OP_swap, DW_OP_xderef, DW_OP_LLVM_fragment 0 32] $vgpr0
 ; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GCN-NEXT:  .Ltmp16:
 ; GCN-NEXT:    .loc 0 28 5 prologue_end ; /tmp/dbg.cl:28:5
 ; GCN-NEXT:    s_setpc_b64 s[30:31]
 ; GCN-NEXT:  .Ltmp17:
+; GCN:    .cfi_endproc
   call void @llvm.dbg.value(metadata i8 addrspace(1)* %arg, metadata !76, metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, DW_OP_xderef)), !dbg !77
   ret i8 addrspace(1)* %arg, !dbg !78
 }
 
 declare void @llvm.dbg.value(metadata, metadata, metadata) #1
 
 attributes #0 = { norecurse nounwind readnone }
 attributes #1 = { nounwind readnone speculatable }

llvm/test/DebugInfo/AMDGPU/cfi.ll

+; RUN: llc -mcpu=gfx900 -mtriple=amdgcn-amd-amdhsa -filetype=obj -o - %s | llvm-dwarfdump -debug-frame - | FileCheck %s
+
+; CHECK: .debug_frame contents:
+; CHECK: 00000000 0000000c ffffffff CIE
+; CHECK-NEXT:   Format:                DWARF32
+; CHECK-NEXT:   Version:               4
+; CHECK-NEXT:   Augmentation:          ""
+; CHECK-NEXT:   Address size:          8
+; CHECK-NEXT:   Segment desc size:     0
+; CHECK-NEXT:   Code alignment factor: 4
+; CHECK-NEXT:   Data alignment factor: 4
+; CHECK-NEXT:   Return address column: 16
+; CHECK-EMPTY:
+; CHECK:   DW_CFA_nop:
+; CHECK-EMPTY:
+; CHECK-NEXT: 00000010 {{[0-9]+}} 00000000 FDE cie=00000000 pc=00000000...{{[0-9]+}}
+; CHECK-NEXT: Format:       DWARF32
+; CHECK-EMPTY:
+; CHECK-EMPTY:
+; CHECK: .eh_frame contents:
+; CHECK-NOT: CIE
+
+define void @func() #0 {
+  ret void
+}
+
+attributes #0 = { nounwind }
+
+!llvm.module.flags = !{!0, !1}
+!llvm.dbg.cu = !{!2}
+
+!0 = !{i32 2, !"Dwarf Version", i32 4}
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, emissionKind: FullDebug)
+!3 = !DIFile(filename: "file", directory: "dir")

llvm/test/MC/ELF/AMDGPU/cfi.s

+# RUN: llvm-mc -filetype=asm -mcpu=gfx900 -triple amdgcn-amd-amdhsa %s -o - | FileCheck --check-prefix=ASM %s
+# RUN: llvm-mc -filetype=obj -mcpu=gfx900 -triple amdgcn-amd-amdhsa %s -o %t
+# RUN: llvm-readelf -S -r -x .debug_frame %t | FileCheck --check-prefix=READELF %s
+
+f:
+  .cfi_sections .debug_frame
+  .cfi_startproc
+  s_nop 0
+  .cfi_endproc
+
+# ASM: f:
+# ASM-NEXT: .cfi_sections .debug_frame
+# NOTE: an extra empty line is an outcome of how AsmParser works today
+# ASM-EMPTY:
+# ASM-NEXT: .cfi_startproc
+# ASM-NEXT: s_nop 0
+# NOTE: an extra empty line is an outcome of how AsmParser works today
+# ASM-EMPTY:
+# ASM-NEXT: .cfi_endproc
+
+# READELF: Section Headers:
+# READELF: Name              Type            Address          Off    Size   ES Flg Lk Inf Al
+# READELF: .debug_frame      PROGBITS        0000000000000000 000048 000028 00      0   0  8
+
+# READELF: Relocation section '.rela.debug_frame' at offset 0xd0 contains 2 entries:
+# READELF-NEXT:     Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
+# READELF-NEXT: 0000000000000014  0000000300000006 R_AMDGPU_ABS32         0000000000000000 .debug_frame + 0
+# READELF-NEXT: 0000000000000018  0000000200000003 R_AMDGPU_ABS64         0000000000000000 .text + 0
+
+# READELF: Hex dump of section '.debug_frame':
+# READELF-NEXT: 0x00000000 0c000000 ffffffff 04000800 04041000 ................
+# READELF-NEXT: 0x00000010 14000000 00000000 00000000 00000000 ................
+# READELF-NEXT: 0x00000020 04000000 00000000                   ........

llvm/test/MC/ELF/AMDGPU/lit.local.cfg

+# We have to reset config.unsupported here because the parent directory is
+# predicated on 'X86'.
+config.unsupported = 'AMDGPU' not in config.root.targets