Page MenuHomePhabricator

[COFF][ARM] Fix CodeView for Windows on 32bit ARM targets.
ClosedPublic

Authored by luqmana on Oct 17 2020, 5:00 AM.

Details

Summary

Create the LLVM / CodeView register mappings for the 32-bit ARM Window targets.

Diff Detail

Event Timeline

luqmana created this revision.Oct 17 2020, 5:00 AM
luqmana requested review of this revision.Oct 17 2020, 5:00 AM
compnerd added inline comments.Oct 19 2020, 9:59 AM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
129–131

I think that Triple.isOSWindows() would be nicer than the explicit check to Win32.

llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
161

It might be nice to change this to explicitly:

case COFF::IMAGE_FILE_MACHINE_AMD64:

and add the x86 case and llvm::fatal_error in any other case.

rnk added a comment.Oct 19 2020, 11:47 AM

Thanks, this is a frequently reported issue.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
129–131

Is this conditional necessary? Can we always use ARMNT? Under what circumstances would one with to emit codeview for a non-windows OS triple?

llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
161

For a dumper, I would prefer that the tool not report a fatal error for an unknown machine type. I think it would be best to send the code to the enumFallback<Hex16> case below, so the registers come out as numbers.

compnerd added inline comments.Oct 19 2020, 1:36 PM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
129–131

The particular case that I think that this is trying to account for is WinCE vs WinNT. However, WinCE isn't currently supported by LLVM, so .... a TODO comment + ARMNT sounds like a good idea to me.

llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
161

Um, lets pretend I never said anything ... your approach is _significantly_ better. Im blaming (lack of) coffee.

luqmana updated this revision to Diff 299237.Oct 19 2020, 6:50 PM

Address review comments.

luqmana marked 6 inline comments as done.Oct 19 2020, 6:50 PM
luqmana added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
129–131

Coment + ARMNT works for me. I just added the conditional there to not break anything existing but if there's no CE support anyways it should be fine.

TomTan added inline comments.Oct 19 2020, 7:57 PM
llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
156

nit: probably assign CPUType in the in switch case, then all calls to getRegisterNames can be consolidated?

luqmana updated this revision to Diff 299253.Oct 19 2020, 9:26 PM
luqmana marked an inline comment as done.

Nit: consolidate getRegisterNames calls.

luqmana marked an inline comment as done.Oct 19 2020, 9:26 PM
compnerd accepted this revision.Oct 19 2020, 9:59 PM

Thanks for doing this!

This revision is now accepted and ready to land.Oct 19 2020, 9:59 PM
This revision was landed with ongoing or failed builds.Oct 19 2020, 10:17 PM
This revision was automatically updated to reflect the committed changes.

Thanks for doing this!

Np, thanks for the review!