Page MenuHomePhabricator

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

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

Diff Detail

Repository
rL LLVM

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 ↗(On Diff #202540)

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

32 ↗(On Diff #202540)

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 ↗(On Diff #202540)

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 ↗(On Diff #202540)

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

32 ↗(On Diff #202540)

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 ↗(On Diff #202540)

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 ↗(On Diff #202540)

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.