[PATCH] D95505: [yaml2obj] Initial support for 32-bit XCOFF in yaml2obj.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:35
+  uint32_t SymbolIndex;
+  uint8_t Size;
+  uint8_t Type;
----------------
Does this name lose the "information" semantics of `r_rsize`?


================
Comment at: llvm/include/llvm/ObjectYAML/XCOFFYAML.h:41
+  StringRef SectionName;
+  uint32_t Address;
+  uint32_t Size;
----------------
uint64_t to make it 64-bit ready?

In ELFYAML.h, we just use the larger type for 32-bit and 64-bit so that no separate definitions are needed.


================
Comment at: llvm/lib/ObjectYAML/XCOFFEmitter.cpp:115
+void XCOFFWriter::writeFileHeader() {
+  // Target machine.
+  W.write<uint16_t>(0x01DF);
----------------
Target machine -> Magic


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:86
   IO.mapRequired("NumberOfSections", FileHdr.NumberOfSections);
   IO.mapRequired("CreationTime", FileHdr.TimeStamp);
   IO.mapRequired("OffsetToSymbolTable", FileHdr.SymbolTableOffset);
----------------
This probably should be mapOptional. f_timdat can be omitted. XCOFFObjectWriter sets it to 0.


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:94
+void MappingTraits<XCOFFYAML::Relocation>::mapping(IO &IO, XCOFFYAML::Relocation &R) {
+  IO.mapRequired("Address", R.VirtualAddress);
+  IO.mapRequired("Symbol", R.SymbolIndex);
----------------
Seems that `r_vaddr` is an offset while called "vaddr". If it can often be 0, make it optional.


================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:103
+  IO.mapRequired("Address", Sec.Address);
+  IO.mapRequired("Size", Sec.Size);
+  IO.mapOptional("FileOffsetToData", Sec.FileOffsetToData);
----------------
In ELF, many fields are printed as hexadecimal so we use `Hex64` a lot. Also, we use `makeOptional` a lot to allow omitting unneeded details.

For Size/SectionData, either one can be specified so it looks like both should be optional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95505