[AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo

[scott.linder]

Summary

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

Add SupportsDebugOnlyCFI as a workaround for targets which do not have
EH support but which do support unwind information for debugging. This
new option only has an effect when the None EH model is specified. The
option requests that .debug_frame be generated when debug info is
requested.

Enable the new option for AMDGPU to prepare for future patches which add
complete CFI support, and to enable testing.

[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

343

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

[echristo]

llvm/include/llvm/MC/MCAsmInfo.h

391

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

[scott.linder]

Rebase

llvm/include/llvm/MC/MCAsmInfo.h

391

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

343

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

343

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

343

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

343

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

343

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