Page MenuHomePhabricator

Implement DW_CFA_LLVM_* for Heterogeneous Debugging
ClosedPublic

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
7

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

clayborg added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
28

Is this the address segment for the address of the row? If so we should change "Address" above to be a:

Optional<object::SectionedAddress> Address;

If this address space qualifies the CFAValue below, then we should move it into UnwindLocation as a "Optional<uint32_t> AddressSpace;" variable

31

You are returning uint8_t but storing this as a CFAAddressSpace as a uint32_t above.

36

Make a constexpr for MaxOperands and use it everywhere so if we bump this number again, we don't need to modify a bunch of places in the code.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
47

not sure printing something like "as2" will be easy for people to know that this means "address space 2". Maybe "address space 2" would be better?

190–191
190–191
250
257
305

" address space %"?

RamNalamothu added inline comments.May 17 2021, 7:52 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
28

Is this the address segment for the address of the row? If so we should change "Address" above to be a:

Optional<object::SectionedAddress> Address;

If this address space qualifies the CFAValue below, then we should move it into UnwindLocation as a "Optional<uint32_t> AddressSpace;" variable

28

No, the address space here qualifies the CFA and specifies, among the different address spaces the target supports, in which address space the relevant stack frame (CFA location) resides. For the sake of understanding this, think of the address spaces as different memory regions, with unique characteristics.

Thought about moving it into UnwindLocation but I noticed that UnwindLocation is being used by both CFA and register rules which, theoretically, making it possible to say CFA's location is CFAPlusOffset and also looks like there aren't enough checks to catch this scenario. Being not sure, I have added it to UnwindRow which has CFAValue.

Does it make sense to split UnwindLocation suitably, so that CFA and register rules could use only what is possible as per DWARF spec?

31

Typo, will fix it. Thank you.

36

Will change it.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
47

Yes.

How about "addrspace 2", which is similar to what LLVM IR/MIR uses?

190–191

Will change.

Addressed the feedback.

This revision is now ready for review again.

clayborg added inline comments.May 18 2021, 11:04 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
28

Thought about moving it into UnwindLocation but I noticed that UnwindLocation is being used by both CFA and register rules which, theoretically, making it possible to say CFA's location is CFAPlusOffset and also looks like there aren't enough checks to catch this scenario. Being not sure, I have added it to UnwindRow which has CFAValue.

UnwindLocation does get used for both CFA and register values, and yes a UnwindLocation does have more values for registers, but since we since CFA values are parsed using special opcodes, there should be no need to worry as those are what creates the right locations.

That being said, I think moving the address space into UnwindLocation seems like the right thing to do here. Nothing that parses the regular register locations should be adding an address space to the location, though I could see this being needed for some registers locations in the future.

Does it make sense to split UnwindLocation suitably, so that CFA and register rules could use only what is possible as per DWARF spec?

I believe the LLDB unwind code does this but I feel this splits up the location code a bit since the CFA usually is register + offset or DWARF expression which are both valid register rules. You could make a CFALocation that inherits from UnwindLocation and adds the address space if that makes things more clear? Then you can also add some asserts when someone tries to set the location to something that the DWARF spec doesn't allow for CFA values.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp
47

That is fine.

ping; is there anyone be able to take a look at the complete patch?

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
28

Thought about moving it into UnwindLocation but I noticed that UnwindLocation is being used by both CFA and register rules which, theoretically, making it possible to say CFA's location is CFAPlusOffset and also looks like there aren't enough checks to catch this scenario. Being not sure, I have added it to UnwindRow which has CFAValue.

UnwindLocation does get used for both CFA and register values, and yes a UnwindLocation does have more values for registers, but since we since CFA values are parsed using special opcodes, there should be no need to worry as those are what creates the right locations.

That being said, I think moving the address space into UnwindLocation seems like the right thing to do here. Nothing that parses the regular register locations should be adding an address space to the location, though I could see this being needed for some registers locations in the future.

Will move address space into UnwindLocation and fit that in the existing way of how locations are handled.

Does it make sense to split UnwindLocation suitably, so that CFA and register rules could use only what is possible as per DWARF spec?

I believe the LLDB unwind code does this but I feel this splits up the location code a bit since the CFA usually is register + offset or DWARF expression which are both valid register rules. You could make a CFALocation that inherits from UnwindLocation and adds the address space if that makes things more clear? Then you can also add some asserts when someone tries to set the location to something that the DWARF spec doesn't allow for CFA values.

I think that will make things more clear. However, since that split up is independent of this patch and the address space changes here are not significant, I think the split up can be done in a different patch, may be a bit later?

clayborg added inline comments.May 19 2021, 3:03 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
28

Just moving the address space to UnwindLocation should be fine. Might be best to use a "Optional<uint32_t> AddressSpace;" as the new ivar in UnwindLocation.

Moved the AddrSpace into UnwindLocation.

clayborg requested changes to this revision.May 21 2021, 3:29 PM
clayborg added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
27–28
27–28
27–28

You could add address space args to this as well, not required though

28–30

Revert and use above constructor

28–31

I don't think we need the "RegPlusOffsetInAddrSpace" enumeration here, we can just use "RegPlusOffset" as the AddrSpace could be used for RegPlusOffset or CFAPlusOffset.

28–31
This revision now requires changes to proceed.May 21 2021, 3:29 PM

Removed RegPlusOffsetInAddrSpace.

RamNalamothu added inline comments.May 26 2021, 6:29 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h
27–28

The current intent is that address space qualifies only the CFA and since CFAPlusOffset refers to just an offset from CFA, probably address space is not needed here.

Skipping it for now.

28–30

Ah, I didn't know earlier that Optional<uint32_t> AS = None is possible.

Done.

28–31

I would prefer to retain the assert to catch any possible unintended use of address space.

I am ok with all of the DWARF unwind changes. Someone else should give the ok for the MC stuff.

Rename few lit test filenames to match with the downstream.

I am ok with all of the DWARF unwind changes. Someone else should give the ok for the MC stuff.

ping: this revision (and the patch series) is under review since March-2020 and would appreciate if someone could review the overall changes and approve.

I am ok with all of the DWARF unwind changes. Someone else should give the ok for the MC stuff.

ping: this revision (and the patch series) is under review since March-2020 and would appreciate if someone could review the overall changes and approve.

Ping

If no one else comments in the next 2 days, I will accept this patch. So speak up soon if you have an opinion.

Thank you @clayborg.

Looks like there aren't going to be any further comments.

clayborg accepted this revision.Jun 11 2021, 3:41 PM

Accepting since no one else had comments. Sorry for the delay.

This revision is now accepted and ready to land.Jun 11 2021, 3:41 PM

No problem.

Thank you very much @clayborg

This revision was automatically updated to reflect the committed changes.