Page MenuHomePhabricator

[AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo
Needs ReviewPublic

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

Details

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

Add a new AsmPrinterHandler called UnwindStreamer which just ensures the
proper .cfi_sections and .cfi_startproc/.cfi_endproc directives are
emitted when the option is in effect. This duplicates trivial amounts of
DwarfException, but not enough to make factoring it out helpful.

In the future this could be unified/simplified with the existing EH
support if debug handling is made orthogonal to unwind information
generation.

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

Diff Detail

Unit TestsFailed

TimeTest
40 mslinux > Clang.CodeGen::lto-newpm-pipeline.c
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/llvm-project/build/lib/clang/12.0.0/include -nostdsysteminc -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null -fexperimental-new-pass-manager -fdebug-pass-manager -flto=full -O0 /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/lto-newpm-pipeline.c 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/clang/test/CodeGen/lto-newpm-pipeline.c -check-prefix=CHECK-FULL-O0
370 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
710 mslinux > LLVM.Other::new-pass-manager.ll
Script: -- : 'RUN: at line 8'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt -disable-output -disable-verify -debug-pass-manager -passes=no-op-module /mnt/disks/ssd0/agent/llvm-project/llvm/test/Other/new-pass-manager.ll 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Other/new-pass-manager.ll --check-prefix=CHECK-MODULE-PASS

Event Timeline

scott.linder created this revision.Apr 23 2020, 6:14 PM

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

@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 added inline comments.May 11 2020, 9:20 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
327

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

echristo added inline comments.
llvm/include/llvm/MC/MCAsmInfo.h
377

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

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 added inline comments.May 12 2020, 7:13 PM
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 added inline comments.May 12 2020, 7:53 PM
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 added inline comments.May 13 2020, 3:40 PM
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 added inline comments.May 13 2020, 6:32 PM
llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
327

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

Rebase and address feedback

Rebase onto LLVM master

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?

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.

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.

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.

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.

Rebase and ping

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.

Rebase onto LLVM Master

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 retitled this revision from Add SupportsDebugUnwindInformation to MCAsmInfo to [AMDGPU] Add SupportsDebugUnwindInformation to MCAsmInfo.
scott.linder edited the summary of this revision. (Show Details)

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 added a comment.EditedFri, Oct 30, 2:25 PM

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.

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