[PATCH] D120858: [NFC][XCOFF] Refactor and format XCOFFObjectWriter.cpp.

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:560
   assert(
-      (TargetObjectWriter->is64Bit() ||
-       Fixup.getOffset() <= UINT32_MAX - Layout.getFragmentOffset(Fragment)) &&
+      (is64Bit() || Fixup.getOffset() <=
+                        MaxRawDataSize - Layout.getFragmentOffset(Fragment)) &&
----------------
do we still need is64Bit()  ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:562
+                        MaxRawDataSize - Layout.getFragmentOffset(Fragment)) &&
       "Fragment offset + fixup offset is overflowed in 32-bit mode.");
   uint32_t FixupOffsetInCsect =
----------------
" in 32-bit mode" will be deleted ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:653
 
-void XCOFFObjectWriter::writeSymbolTableEntryForCsectMemberLabel(
-    const Symbol &SymbolRef, const XCOFFSection &CSectionRef,
-    int16_t SectionIndex, uint64_t SymbolOffset) {
-  // Name or Zeros and string table offset
-  writeSymbolName(SymbolRef.getSymbolTableName());
-  assert(SymbolOffset <= UINT32_MAX - CSectionRef.Address &&
-         "Symbol address overflows.");
-  W.write<uint32_t>(CSectionRef.Address + SymbolOffset);
-  W.write<int16_t>(SectionIndex);
+void XCOFFObjectWriter::writeSymbolBody(StringRef SymbolName, uint64_t Value,
+                                        int16_t SecIdx, uint16_t Type,
----------------
change writeSymbolBody to writeSymbolEntry ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:655
+                                        int16_t SecIdx, uint16_t Type,
+                                        uint8_t StorageClass, uint8_t AuxCnt) {
+  writeSymbolName(SymbolName); // n_name, n_zeros, n_offset
----------------
do we want to set a default AuxCnt=1 ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:668
+  W.write<uint8_t>(StorageClass); // n_sclass
   // Always 1 aux entry for now.
+  W.write<uint8_t>(AuxCnt); // n_numaux
----------------
delete  the comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:672
+
+void XCOFFObjectWriter::writeAuxSymbolCsect(uint64_t SecLen, uint8_t Type,
+                                            uint8_t MapClass) {
----------------
change Name to writeSymbolAuxCsectEntry ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:674
+                                            uint8_t MapClass) {
+  W.write<uint32_t>(Lo_32(SecLen)); // x_scnlen
+  W.write<uint32_t>(0);             // x_parmhash
----------------
I am not sure which ( "x_scnlen" or "SectionOrLength")one is better for comment ?   maybe the SectionOrLength from XCOFFObjectFile.h is better.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:683
+
+void XCOFFObjectWriter::writeAuxSymbolDwarf(uint64_t SecLen,
+                                            uint64_t RelCnt = 0) {
----------------
function name  change to writeSymbolAuxDwarfEntry ?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:726
 void XCOFFObjectWriter::writeFileHeader() {
-  // Magic.
-  W.write<uint16_t>(0x01df);
-  // Number of sections.
-  W.write<uint16_t>(SectionCount);
-  // Timestamp field. For reproducible output we write a 0, which represents no
-  // timestamp.
-  W.write<int32_t>(0);
-  // Byte Offset to the start of the symbol table.
-  W.write<uint32_t>(SymbolTableOffset);
-  // Number of entries in the symbol table.
-  W.write<int32_t>(SymbolTableEntryCount);
-  // Size of the optional header.
-  W.write<uint16_t>(0);
-  // Flags.
-  W.write<uint16_t>(0);
+  W.write<uint16_t>(is64Bit() ? XCOFF::XCOFF64 : XCOFF::XCOFF32);
+  W.write<uint16_t>(SectionCount);         // f_nscns
----------------
since the header is for 32bit. 
  W.write<uint32_t>(SymbolTableOffset);    // f_symptr
  W.write<int32_t>(SymbolTableEntryCount); // f_nsyms

we should modify the code 
as 
assert(!is64Bit() && "not support 64 bit for writing file header.");
W.write<uint16_t>(XCOFF::XCOFF32);


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:756
 
-    W.write<uint32_t>(Sec->Size);
-    W.write<uint32_t>(Sec->FileOffsetToData);
-    W.write<uint32_t>(Sec->FileOffsetToRelocations);
-
-    // Line number pointer. Not supported yet.
-    W.write<uint32_t>(0);
-
-    W.write<uint16_t>(Sec->RelocationCount);
-
-    // Line number counts. Not supported yet.
-    W.write<uint16_t>(0);
+    W.write<uint32_t>(Sec->Size);                    // s_size
+    W.write<uint32_t>(Sec->FileOffsetToData);        // s_scnptr
----------------
I do not think we need the s_size here , Sec->Size can self-explain.
ditto for the following.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120858