[PATCH] D103696: [XCOFF][AIX] Add support for XCOFF 64 bit Object files

[EsmeYi via Phabricator via llvm-commits]

Esme added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:280
 
+struct FileHeader64 {
+  uint16_t Magic;
----------------
jhenderson wrote:
> Let's keep the 32 bit and 64 bit versions of each of these structs next to each other, so the order will be: `FileHeader32`, `FileHeader64`, `SectionHeader32`, `SectionHeader64`.
Please refer to patch D103901, we have removed these structures as we only use their size. It looks like this patch also only use their `sizeof`, so it's reasonable to replace the two structs with the constants:

```
constexpr size_t FileHeaderSize64 = 24;
constexpr size_t SectionHeaderSize64 = 72;
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:206
 
-  static bool nameShouldBeInStringTable(const StringRef &);
+  bool nameShouldBeInStringTable(const StringRef &);
   void writeSymbolName(const StringRef &);
----------------
> StringRef is small and pervasive enough in LLVM that it should always be passed by value. (https://bcain-llvm.readthedocs.io/projects/llvm/en/latest/ProgrammersManual/#the-stringref-class)


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:580
 bool XCOFFObjectWriter::nameShouldBeInStringTable(const StringRef &SymbolName) {
-  return SymbolName.size() > XCOFF::NameSize;
+  return (SymbolName.size() > XCOFF::NameSize) || TargetObjectWriter->is64Bit();
 }
----------------
`nameShouldBeInStringTable()` will never be called if the `TargetObjectWriter->is64Bit()` is true, right? Maybe we can just remove the or condition.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:745
     // Write the Physical Address and Virtual Address. In an object file these
     // are the same.
+    if (TargetObjectWriter->is64Bit()) {
----------------
Move this comment to the appropriate code, ie. inside the if.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern-weak.ll:336
+
+; CHECKSYM64: Symbols [
+; CHECKSYM64-NEXT:   Symbol {
----------------
Add some space to keep alignment. Same as below.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-return55.ll:94
+
+;CHECKSECT64: Sections [
+;CHECKSECT64-NEXT:   Section {
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:47
   void printRelocations(ArrayRef<XCOFFSectionHeader32> Sections);
+  void printRelocations64(ArrayRef<XCOFFSectionHeader64> Sections);
   const XCOFFObjectFile &Obj;
----------------
I think you should split these part to another patch about implementation of printing relocations64 in llvm-readobj, and add corresponding test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103696