Page MenuHomePhabricator

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.

Diff Detail

Unit TestsFailed

TimeTest
9,840 mslinux > libomp.env::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && env KMP_DISP_NUM_BUFFERS=0 /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp
1,800 mslinux > libomp.worksharing/for::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

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
328

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

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

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

Rebase

llvm/include/llvm/MC/MCAsmInfo.h
359

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
328

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
328

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
328

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
328

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
328

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

Rebase and address feedback