[PATCH] D89622: [COFF][ARM] Fix CodeView for Windows on 32bit ARM targets.

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:129
   case Triple::ArchType::thumb:
-    return CPUType::Thumb;
+    return Triple.getOS() == Triple::Win32 ? CPUType::ARMNT : CPUType::Thumb;
   case Triple::ArchType::aarch64:
----------------
rnk wrote:
> compnerd wrote:
> > I think that `Triple.isOSWindows()` would be nicer than the explicit check to Win32.
> Is this conditional necessary? Can we always use ARMNT? Under what circumstances would one with to emit codeview for a non-windows OS triple?
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.


================
Comment at: llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp:161
+    break;
+  default:
+    CpuType = CPUType::X64;
----------------
rnk wrote:
> compnerd wrote:
> > 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.
> 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.
Um, lets pretend I never said anything ... your approach is _significantly_ better.  Im blaming (lack of) coffee.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89622/new/

https://reviews.llvm.org/D89622