Page MenuHomePhabricator

Implement DW_CFA_LLVM_* for Heterogeneous Debugging
Needs ReviewPublic

Authored by scott.linder on Mar 26 2020, 12:16 PM.

Details

Summary

Add support in MC/MIR for writing/parsing, and DebugInfo.

This is part of the Extensions for Heterogeneous Debugging defined at
https://llvm.org/docs/AMDGPUDwarfExtensionsForHeterogeneousDebugging.html

Specifically the CFI instructions implemented here are defined at
https://llvm.org/docs/AMDGPUDwarfExtensionsForHeterogeneousDebugging.html#cfa-definition-instructions

Diff Detail

Unit TestsFailed

TimeTest
410 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
100 mslinux > MLIR.Target::llvmir.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-translate -mlir-to-llvmir -split-input-file /mnt/disks/ssd0/agent/llvm-project/mlir/test/Target/llvmir.mlir | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/mlir/test/Target/llvmir.mlir
890 mslinux > libomp.ompt/tasks::dependences.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/ompt/tasks/dependences.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/ompt/tasks/Output/dependences.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/ompt/tasks/Output/dependences.c.tmp | sort -n -s | tee /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/ompt/tasks/Output/dependences.c.tmp.out | /mnt/disks/ssd0/agent/llvm-project/build/./bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt/tasks/dependences.c
30 mswindows > Clang-Unit.Format/_/FormatTests_exe::TokenAnnotatorTest.UnderstandsUsesOfStarAndAmpInMacroDefinition
Note: Google Test filter = TokenAnnotatorTest.UnderstandsUsesOfStarAndAmpInMacroDefinition [==========] Running 1 test from 1 test case.
100 mswindows > MLIR.Target::llvmir.mlir
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\mlir-translate.exe -mlir-to-llvmir -split-input-file C:\ws\w16n2-1\llvm-project\premerge-checks\mlir\test\Target\llvmir.mlir | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\mlir\test\Target\llvmir.mlir

Event Timeline

scott.linder created this revision.Mar 26 2020, 12:16 PM

git clang-format

The new operation is defined at https://llvm.org/docs/AMDGPUUsage.html#cfa-definition-instructions. @t-tye is working on formatting the proposal to be sent to upstream DWARF as an RFC, and I am working to land these patches as an initial implementation.

Update to match changes in proposal.

Update encodings of DW_OP_CFA_* to avoid conflict with a GNU extension.

scott.linder retitled this revision from Implement DW_CFA_LLVM_def_cfa_aspace to Implement DW_CFA_LLVM_* for Heterogeneous Debugging.
scott.linder edited the summary of this revision. (Show Details)

Update docs for encoding change

RamNalamothu added inline comments.May 5 2020, 6:33 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
36

Now, maximum number of operands is 3

llvm/lib/MC/MCDwarf.cpp
1446

Emitting DW_CFA_def_aspace_cfa_sf is yet to be handled?

I see that even the emission of DWARF 5 supported DW_CFA_def_cfa_sf and DW_CFA_def_cfa_offset_sf are also yet to be handled.

scott.linder marked 2 inline comments as done.May 8 2020, 2:51 PM
scott.linder added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
36

Thank you, I'll update the patch.

llvm/lib/MC/MCDwarf.cpp
1446

Right, this should probably be a TODO in the code. As the existing _sf variants are not implemented yet either and AMDGPU does not currently require any of them (we always retain a copy of the stack pointer on entry in some form) I didn't implement it here.

Address feedback

arsenm added a subscriber: arsenm.May 12 2020, 11:51 AM
arsenm added inline comments.
llvm/lib/CodeGen/MIRParser/MILexer.cpp
215–218

Unrelated change

236–238

Unrelated change

llvm/lib/MC/MCParser/AsmParser.cpp
4195

Tests missing for the parse errors?

scott.linder marked an inline comment as done.
scott.linder removed a subscriber: arsenm.

Add tests for parse errors, fix formatting

llvm/lib/CodeGen/MIRParser/MILexer.cpp
215–218

I wish all of this rote mechanical stuff were just automated. Do you have a preference on how I resolve this? The options I'm aware of:

  • I can factor out every clang-format diff into a separate NFC review, but effectively every non-trivial patch would need one, so I would end up having to manage on the order of twice as many Phabricator reviews.
  • I can make one pass at the very end to try to pull all the unrelated NFC formatting changes into one commit, but I would rather do this just before committing so I am not constantly having to comb through every patch in the set trying to clean them up.
  • I can aggressively commit these NFC cleanups as separate patches without reviews as they come up, before posting the actual review.

Or I can lobby on llvm-dev to just lint/format the whole codebase once, and then gate commits on patches not breaking it. This would probably save everyone too much time, and make git blame too useful though.

scott.linder marked an inline comment as done.May 13 2020, 2:09 PM
dblaikie added inline comments.May 13 2020, 3:20 PM
llvm/lib/CodeGen/MIRParser/MILexer.cpp
215–218

Most of it is automated or can be - if you're only clang-formatting the lines of code you've edited, it's usually enough - just in this instance happens to be a very long statement & so it gets reformatted together even if you changed unrelated lines in the statement. /maybe/ clang-format could be modified to not do that (to be able to reformat only the edited lines, rather than the whole statement)

Something like this one - yeah, you could commit that specific formatting (not reformatting the whole file) in advance to ensure the review is simpler/clearer. I think this fits under the general idea of reformatting early if you're making significant changes - like if you're majorly reworking a whole file, it's not uncommon to reformat the whole file. But yeah, if you're touching a statement/block in depth and it's significantly misformatted, easy thing to fix/commit in advance - doesn't need a formal pre-commit review (so long as it's code that's going to be touched/changed soon - don't go reformatting whole files without such motivation, unless they're really egregiously formatted).

scott.linder marked an inline comment as done.May 13 2020, 4:32 PM
scott.linder added inline comments.
llvm/lib/CodeGen/MIRParser/MILexer.cpp
215–218

Is there any document with the rationale for why LLVM prefers incrementally massaging code into the canonical format, rather than just fixing the entire codebase at once? Like a section in some docs somewhere, or an old llvm-dev thread? It might make the pill easier to swallow if I understand the ideal to which I'm dedicating time fiddling with it.

Use llvm-readelf for more compact tests

Correct test file names. I missed this when renaming the CFA operation itself
to match the updated proposal.

Harbormaster completed remote builds in B56771: Diff 264061.

Rebase and address feedback.

Rebase and update CFIProgram::parse (DWARFDebugFrame.cpp) changes as
per https://reviews.llvm.org/rG1e820e82b1438a52124512175a0e7c6f8d23e158

Rebase onto LLVM master

Rebase, update x86 test (dwarfdump now prefers register names for some
targets), update link to the extensions in a comment.

Also, ping; is there anyone able to take a look?

The new operation is defined at https://llvm.org/docs/AMDGPUUsage.html#cfa-definition-instructions. @t-tye is working on formatting the proposal to be sent to upstream DWARF as an RFC, and I am working to land these patches as an initial implementation.

Can you link the documentation in the summary? Currently https://llvm.org/docs/AMDGPUUsage.html does not mention DW_CFA_LLVM_

llvm/lib/MC/MCDwarf.cpp
1446

The variables are used only once and can be inlined

1449

The negative offset looks strange.

Before May, there was a double negation problem which has been fixed by 0840d725c4e7c98bb440db7b886054525be6ddf1

llvm/test/MC/ELF/cfi-llvm-def-aspace-cfa-errors.s
8

[[@LINE]] is deprecated syntax. Please use [[#@LINE+1]] for new tests.

scott.linder edited the summary of this revision. (Show Details)

Address review comments:

  • Include link to extension docs in commit message
  • Eliminate some variables
  • Fix double-negation in createLLVMDefAspaceCfa
  • Fix use of some deprecated lit syntax
scott.linder marked 3 inline comments as done.Oct 9 2020, 10:34 AM

The new operation is defined at https://llvm.org/docs/AMDGPUUsage.html#cfa-definition-instructions. @t-tye is working on formatting the proposal to be sent to upstream DWARF as an RFC, and I am working to land these patches as an initial implementation.

Can you link the documentation in the summary? Currently https://llvm.org/docs/AMDGPUUsage.html does not mention DW_CFA_LLVM_

Thank you for the review! I included the updated link directly to the documentation for these new instructions in the commit message.

scott.linder edited the summary of this revision. (Show Details)

Tweak which links are present where: link directly to the description of the
new CFI operations in the source, and put both links in the commit message.

Rebase on top of LLVM master