[PATCH] D128148: [XCOFF] write the aux header when the visibility is specified in XCOFF32.

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:34
 constexpr size_t AuxFileHeaderSize64 = 110;
+constexpr size_t AuxFileHeaderSizeObj = 28;
 constexpr size_t SectionHeaderSize32 = 40;
----------------
not sure how the size 28 come from ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:309
   size_t auxiliaryHeaderSize() const {
-    assert(!needsAuxiliaryHeader() &&
-           "Auxiliary header support not implemented.");
-    return 0;
+    return HasVisibility && !is64Bit() ? XCOFF::AuxFileHeaderSizeObj : 0;
   }
----------------
according to "The auxiliary header contains system-dependent and implementation-dependent information, in which is used for loading and executing a module. Information in the auxiliary header minimizes how much of the file must be processed by the system loader at execution time." in  https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__fyovh386shar

the auxiliaryHeaderSize should not depend on the HasVisibility 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:774
+  W.write<uint16_t>(0); // Magic
+  W.write<uint16_t>(2); // Version. The new interpretation of the n_type
+                        // field in the symbol table entry is used in XCOFF32.
----------------
in the xcoff.h , there is 
enum XCOFFInterpret : uint16_t {
  OLD_XCOFF_INTERPRET = 1,
  NEW_XCOFF_INTERPRET = 2
};


maybe better to use NEW_XCOFF_INTERPRET  than 2 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128148