[PATCH] D137819: [XCOFF] support the overflow section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

The functionality of this patch is best left for @DiggerLin to review, so I've kept my comments to stylistic ones.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:252
   std::vector<DwarfSectionEntry> DwarfSections;
+  std::vector<SectionEntry> OvrfloSections;
 
----------------
Not sure why you're abbreviating here. Just use `OverflowSections`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:354
     DwarfSec.reset();
+  for (auto &OvrfloSec : OvrfloSections)
+    OvrfloSec.reset();
----------------
Again, `OverflowSec` would be preferable.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:812
     // Write the Physical Address and Virtual Address. In an object file these
-    // are the same.
+    // are the same, except for the the overflow section header.
+    // In the overflow section header the Physical Address specifies the number
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:815
+    // of relocation entries actually required and the Virtual Address specifies
+    // the number of line-number entries actually required.
     // We use 0 for DWARF sections' Physical and Virtual Addresses.
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:818
     writeWord(IsDwarf ? 0 : Sec->Address);
-    writeWord(IsDwarf ? 0 : Sec->Address);
+    // Since line number is not supported, we set it to 0 for overflow section.
+    writeWord((IsDwarf || IsOvrflo) ? 0 : Sec->Address);
----------------
Either "for the overflow section" or "for overflow sections".


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:834
+      // primary section header and s_nlnno must have the same value.
+      // For common section header, either of s_nreloc or s_nlnno is set to
+      // 65535, the other one must also be set to 65535.
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:942
+
+  auto countRelocations = [&](SectionEntry *Sec, uint64_t RelCount) {
+    // Handles relocation field overflows in an XCOFF32 file. An XCOFF64 file
----------------
General consensus is that lambdas should be named like variables, because they are function objects, not functions. I.e. `CountRelocations`, not `countRelocations`.

That being said, this lambda is to long. Pull it out into a helper function instead.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:945
+    // may not contain an overflow section header.
+    if (!is64Bit() && (RelCount >= (uint32_t)XCOFF::RelocOverflow)) {
+      // Generate an overflow section header.
----------------
Nit: prefer `static_cast` to C-style cast.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:957
+
+      SectionCount++;
+      SecEntry.Index = SectionCount;
----------------
https://llvm.org/docs/CodingStandards.html#prefer-preincrement

You should probably also do it as part of the assignment, i.e. `SecEntry.Index = ++SectionCount;`


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1000
+    if (RawPointer > MaxRawDataSize)
+      report_fatal_error("Section raw data overflowed this object file.");
+
----------------
Noting here and in other places that this should really return an `Error` rather than `report_fatal_error`. That's another piece of work though.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1007
+  // alignment, while DWARF sections have their own alignments. If these two
+  // alignments are not the same, we need some paddings here and record the
+  // paddings bytes for FileOffsetToData calculation.
----------------
Simply "some padding" or "some padding bytes", but not "some paddings".

Also I'd change the last bit to "and to use this padding in FileOffsetToData calculation."


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1023
+  // Calculate the file offset to the relocation entries.
+  auto calcOffsetToRelocations = [&](SectionEntry *Sec) {
     if (!Sec->RelocationCount)
----------------
Again, this lambda is too long - pull it into a helper function.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1029
+    uint64_t RelocationSizeInSec = 0;
+    if (!is64Bit() && Sec->RelocationCount == (uint32_t)XCOFF::RelocOverflow) {
+      // Find its corresponding overflow section.
----------------
Prefer `static_cast`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1032
+      for (auto &OvrfloSec : OvrfloSections) {
+        if (OvrfloSec.RelocationCount == (uint32_t)Sec->Index) {
+          RelocationSizeInSec =
----------------
Prefer `static_cast`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137819