[PATCH] D122287: [XCOFF][2/3] support writing sections and relocations for XCOFF64.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

My only real comment on this is patch ordering. An XCOFF/AIX person should review too.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:749
     W.write<uint16_t>(0); // Flags
-    W.write<int32_t>(0);  // SymbolTableEntryCount. Not supported yet.
+    W.write<int32_t>(1);  // SymbolTableEntryCount. Not supported yet.
   } else {
----------------
The non-zero value plus "not supported yet" comment doesn't quite work. Perhaps "Only source file symbol supported"?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:808-809
   }
-  W.write<uint32_t>(Reloc.SymbolTableIndex);
+  // TODO SymbolTable for XCOFF64 is not yet supported.
+  W.write<uint32_t>(is64Bit() ? 0 : Reloc.SymbolTableIndex);
   W.write<uint8_t>(Reloc.SignAndSize);
----------------
I'd have thought implementing the symbol table before relocations makes more sense.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:847-850
+  // TODO Writing 64-bit symbol table is not yet supported. Only one entry is
+  // currently being written, in order to verify the functionality of
+  // 64-bit relocations.
+  if (is64Bit())
----------------
Same comment as above: support the symtab first?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122287