This is an archive of the discontinued LLVM Phabricator instance.

[COFF, ARM64] Fix CodeView API change for getRegisterNames
ClosedPublic

Authored by TomTan on Jun 1 2019, 12:46 AM.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

TomTan created this revision.Jun 1 2019, 12:46 AM

Are you sure this triple → CPUType mapping belongs in each consumer? Maybe it'd be better to have something inside LLVM, so that we wouldn't have to keep this up-to-date in all the places. Maybe getRegisterNames() overload that takes a triple?

TomTan updated this revision to Diff 202540.Jun 1 2019, 1:06 AM
TomTan edited the summary of this revision. (Show Details)

Update variable naming to be consistent with current file.

TomTan added a comment.Jun 1 2019, 1:17 AM

Are you sure this triple → CPUType mapping belongs in each consumer? Maybe it'd be better to have something inside LLVM, so that we wouldn't have to keep this up-to-date in all the places. Maybe getRegisterNames() overload that takes a triple?

The CodeView APIs seems not aware of LLVM tripe. CPUType comes from CodeView which makes this API self-consistent.

D62771 does address the same.

aganea added a subscriber: aganea.Jun 2 2019, 7:04 AM
aganea added inline comments.
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
29

Shouldn’t ArchType::aarch64_be and ArchType::aarch64_32 enums be handled here as well?

32

Intel80386 for consistency?

compnerd requested changes to this revision.Jun 2 2019, 10:46 AM
compnerd added a subscriber: compnerd.
compnerd added inline comments.
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
29

I think that we should use a switch to cover the targets. /Oy will allow FPO on x86 as well. There is also WoA (ARM32).

This revision now requires changes to proceed.Jun 2 2019, 10:46 AM
TomTan marked an inline comment as done.Jun 2 2019, 10:49 AM
TomTan added inline comments.
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
29

Seems no, aarch64_be or aarch64_32 is not supported by CodeView or Windows.

32

x86 and x64 are handled as the same CPUType by CodeView. LLVM pdb dump tool also sets X64 as default.

TomTan updated this revision to Diff 202614.Jun 2 2019, 10:55 AM

Change to switch/case based on comment.

TomTan marked an inline comment as done.Jun 2 2019, 10:57 AM
TomTan added inline comments.
source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
29

Ok. It makes sense to use switch/case. CodeView doesn't support WoA (ARM32) so no need to add case for it here.

compnerd accepted this revision.Jun 2 2019, 5:29 PM

This should get the build working again, so lets get this fixed, we can improve it later

source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp
29

Huh? How does cl generate that then?

This revision is now accepted and ready to land.Jun 2 2019, 5:29 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2019, 5:47 PM
TomTan added a comment.Jun 2 2019, 5:51 PM

This should get the build working again, so lets get this fixed, we can improve it later

Thanks, committed the fix to unblock build. For WoA(ARM32) which is not supported, I meant LLVM CodeView, not CodeView specification. llvm::codeview::getRegisterNames(cpu_type) will return incorrect register name even the right CPUType is passed in for WoA(ARM32), because it has not been implemented in LLVM CodeView yet.