Page MenuHomePhabricator

[MCAsmInfo] Support UsesCFIForDebug for targets with no exception handling
ClosedPublic

Authored by RamNalamothu on Apr 23 2020, 6:14 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
scott.linder edited the summary of this revision. (Show Details)

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

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 added a comment.EditedDec 11 2020, 10:54 AM

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 added a comment.EditedDec 11 2020, 11:10 AM

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.

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.

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?

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)

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.

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.

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

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

Rebase and ping

RamNalamothu retitled this revision from [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo to [AMDGPU] Add SupportsDebugOnlyCFI to MCAsmInfo.Mar 9 2021, 12:56 AM
RamNalamothu edited the summary of this revision. (Show Details)

Rebase onto ToT.

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

Rebase and ping

dblaikie added inline comments.Apr 2 2021, 12:16 PM
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
428

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
367–369

Minor thing, I think this can be written more simply

RamNalamothu commandeered this revision.Apr 26 2021, 8:22 AM
RamNalamothu retitled this revision from [AMDGPU] Add SupportsDebugOnlyCFI to MCAsmInfo to [AsmPrinter] Fix emitting CFI for debug when exceptions are not supported.
RamNalamothu edited the summary of this revision. (Show Details)
RamNalamothu added a reviewer: MaskRay.
RamNalamothu edited reviewers, added: scott.linder; removed: RamNalamothu.

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

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

RamNalamothu marked 3 inline comments as done.Apr 26 2021, 8:48 AM
dblaikie accepted this revision.Apr 26 2021, 11:37 AM

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

This revision is now accepted and ready to land.Apr 26 2021, 11:37 AM

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

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 added a comment.EditedApr 27 2021, 11:45 AM
This comment has been deleted.
llvm/include/llvm/CodeGen/AsmPrinter.h
378

"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

ExceptionsType is not mentioned.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
354

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

1071

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

Why can't it use DwarfCFI?

RamNalamothu added inline comments.Apr 27 2021, 12:29 PM
llvm/include/llvm/CodeGen/AsmPrinter.h
378

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

ExceptionsType is not mentioned.

Sorry, I didn't understand that.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
354

Yes, will change it to

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

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 added inline comments.Apr 27 2021, 12:40 PM
llvm/include/llvm/CodeGen/AsmPrinter.h
378

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
354

Adding a blank line below None works as well.

1071

Ah, ok, now I understand this part.

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 added inline comments.Apr 27 2021, 2:10 PM
llvm/test/MC/WebAssembly/debug-info.ll
172 ↗(On Diff #340842)

+ @sbc100 for wasm .debug_frame change.

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

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.

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 retitled this revision from [AsmPrinter] Fix emitting CFI for debug when exceptions are not supported to [MCAsmInfo] Support UsesCFIForDebug for targets with no exception handling.May 5 2021, 7:44 AM
RamNalamothu edited the summary of this revision. (Show Details)
RamNalamothu marked 12 inline comments as done.

Does it look good now?

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

MaskRay accepted this revision.May 5 2021, 10:11 AM
MaskRay added inline comments.
llvm/test/CodeGen/AMDGPU/debug_frame.ll
0–1

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.

Address feedback on LIT tests.

RamNalamothu marked 2 inline comments as done.May 5 2021, 11:22 AM

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

Thank you.

MaskRay accepted this revision.May 5 2021, 11:29 AM

LGTM, with one last nit

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

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

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

RamNalamothu marked an inline comment as done.May 5 2021, 12:21 PM