This is an archive of the discontinued LLVM Phabricator instance.

[ARM64, COFF] Add CodeView register mapping
ClosedPublic

Authored by TomTan on May 29 2019, 9:10 AM.

Details

Summary

CodeView has its own register map which is defined in cvconst.h. Missing this mapping before saving register to CodeView causes debugger to show incorrect value for all register based variables, like variables in register and local variables addressed by register (stack pointer + offset).

This change added mapping between LLVM register and CodeView register so the correct register number will be stored to CodeView/PDB, but it doesn't fix the translation from CodeView register number to register name, because CodeView register numbers overlap between architectures. So the tools like llvm-pdbutil need extra fix in order to dump ARM64 registers correctly from PDB.

Diff Detail

Repository
rL LLVM

Event Timeline

TomTan created this revision.May 29 2019, 9:10 AM
efriedma added inline comments.May 29 2019, 11:04 AM
include/llvm/DebugInfo/CodeView/CodeViewRegisters.def
354 ↗(On Diff #201942)

It's worth taking a moment to figure out how this will generalize beyond the current patch.

Probably the right approach is to provide CV_REGISTER_ARM64, CV_REGISTER_X86, and CV_REGISTER_ALL as possibilities, where CV_REGISTER_X86 only provides the x86 entries, CV_REGISTER_ARM64 only provides the ARM64 entries, and CV_REGISTER_ALL provides the entries for all targets. That would make it clear in each place that includes CodeViewRegisters.def which registers are actually being used.

mstorsjo added inline comments.
include/llvm/DebugInfo/CodeView/CodeViewRegisters.def
430 ↗(On Diff #201942)

Typo, statue/status

TomTan updated this revision to Diff 202032.May 29 2019, 1:07 PM
TomTan added a reviewer: mstorsjo.

Added CV_REGISTERS_{ARCH} macro for each architecture, and fixed typo.

The code looks fine now. I'm okay with putting off fixes for dumping code that incorrectly assumes x86.

test/DebugInfo/COFF/register-variables-arm64.ll
87 ↗(On Diff #202032)

Do you really need two functions here, given what you're testing?

Please try to clean up the testcase IR as much as you can. I know you probably don't want to manually edit debug metadata, since it's easy to accidentally write incorrect debug info that way, but please do what you can for the other irrelevant bits, like attributes and the llvm.ident metadata.

TomTan updated this revision to Diff 202053.May 29 2019, 2:18 PM

Did a little cleanup as per Eli's comment (removed attributes and llvm.ident. Tried some others but got llc failure, so I'd like to be conservation on this. Also the test follows register-variables.ll in the same folder which keeps all the metadata.

rnk added a comment.May 29 2019, 2:32 PM

The code looks fine now. I'm okay with putting off fixes for dumping code that incorrectly assumes x86.

The dumping tools are the foundation of our testing strategy, so I think we should take the time to fix them early on. It should be a matter of changing getRegisterNames to accept a CPU type, switch, and return the appropriate (x86 or arm) enum table, maybe just returning x86 by default to preserve existing behavior. Both symbol dumpers already store the compilation CPU type from the S_COMPILE3 record, so this shouldn't be too complicated.

test/DebugInfo/COFF/register-variables-arm64.ll
26 ↗(On Diff #202053)

So, you have to wild card the register here because the dumper assumes x86 registers. I think we should take the time to fix that now before we accumulate more tests.

In D62608#1522078, @rnk wrote:

The code looks fine now. I'm okay with putting off fixes for dumping code that incorrectly assumes x86.

The dumping tools are the foundation of our testing strategy, so I think we should take the time to fix them early on. It should be a matter of changing getRegisterNames to accept a CPU type, switch, and return the appropriate (x86 or arm) enum table, maybe just returning x86 by default to preserve existing behavior. Both symbol dumpers already store the compilation CPU type from the S_COMPILE3 record, so this shouldn't be too complicated.

Yes, I was thinking on passing CPU arch (as you mentioned from S_COMPILE3) to get correct register name. I am trying to find a way in each include site of "CodeViewRegisters.def" to get that information. At the mean time, the current issue affects debugging experience on Chromium and derived projects. As register name query only affects LLVM pdb tools, could this change be merged at first while I am working on fixing LLVM pdb tool? Also ARM64 support for LLVM pdb tool is new, so it would not block anything or test.

rnk added a comment.May 29 2019, 3:10 PM

Yes, I was thinking on passing CPU arch (as you mentioned from S_COMPILE3) to get correct register name. I am trying to find a way in each include site of "CodeViewRegisters.def" to get that information. At the mean time, the current issue affects debugging experience on Chromium and derived projects. As register name query only affects LLVM pdb tools, could this change be merged at first while I am working on fixing LLVM pdb tool? Also ARM64 support for LLVM pdb tool is new, so it would not block anything or test.

The way I interpret LLVM's project policy is that new features and bug fixes should have tests, and with our testing infrastructure, that usually means starting with a dumper. Right now I'm working on missing inline line tables, and before I send the functionality for review and write tests for it, I've gone and improved the dumper in rL362037.

A while back (rL312042) I rebased an improvement to llvm-dwarfdump to do this kind of location pretty printing stuff, and when I did, I uncovered several of these kinds of tests where the dumper printed the hex bytes of a dwarf expression, and the test contained comments explaining that these bytes were correct. However, the comments were stale and the bytes were actually wrong, at some point someone updated them without interpreting the bytes and checking the comments. I'd like to avoid similar situations in the future. :)

TomTan updated this revision to Diff 202466.May 31 2019, 12:12 PM

It makes sense to fix the dumper for test infrastructure. This update fixes register names in the dumper. I also changed the wildcard for register name to ARM64_SP in the new test, because register name is shown correct by the dumper based on CPUType in the PDB.

There is still a TODO item for printing to yaml. I haven't found a easy way to pass in CPUType for register name query. I'd like to get your advise on this and fix it later.

rnk accepted this revision.May 31 2019, 1:11 PM

lgtm, thanks!

lib/DebugInfo/PDB/Native/NativeSession.cpp
86 ↗(On Diff #202466)

Did you mean to add const here? It doesn't seem necessary, please revert it if so.

This revision is now accepted and ready to land.May 31 2019, 1:11 PM
TomTan marked 2 inline comments as done.May 31 2019, 2:17 PM
TomTan added inline comments.
lib/DebugInfo/PDB/Native/NativeSession.cpp
86 ↗(On Diff #202466)

Thanks for the catch. Yes, this is unnecessary. I tried query CPUType from getGlobalScope(), but found a class variable CompilationCPU for that. I'll revert the all added const for getLobalScope().

TomTan updated this revision to Diff 202490.May 31 2019, 2:48 PM
TomTan marked an inline comment as done.

Remove unnecessary const and clean up.

This revision was automatically updated to reflect the committed changes.
TomTan marked an inline comment as done.
mgorny added a comment.Jun 2 2019, 8:33 AM

Could we, please, revert this until the fix for LLDB is ready and approved?

Could we, please, revert this until the fix for LLDB is ready and approved?

The fix for LLDB was merged yesterday.