[NFC][XCOFF] Use common function to calculate file offset

[Jake-Egan]

Summary

The file offset code is repeated in nearly identical form for every derivation of SectionEntry, so make it into a method in SectionEntry instead.

[scott.linder]

llvm/lib/MC/XCOFFObjectWriter.cpp

125

Generally best to pass primitive scalars like this by value when they are read-only

125

As there are already virtual methods I don't think the cost of making this virtual is too high, and it is what I would expect. I'd be surprised if a call to SectionEntry::advanceFileOffset was just wrong if the object were a DwarfSectionEntry.

Alternatively this method could make a call to a virtual method which returns the "RawSize" of the section, which appears to be different for DwarfSectionEntry (i.e. for every other section it is Size, for DwarfSectionEntry it is MemorySize)

193

I'm not sure why DwarfSectionEntry doesn't have a reset override which also clears MemorySize?

[Jake-Egan]

Update patch

[stephenpeckham]

llvm/lib/MC/XCOFFObjectWriter.cpp

125

Would it make more sense for this function to return the new RawPointer instead of using reference to the variable?

201

I think this special treatment of Dwarf sections is unnecessary, but changing it probably needs a separate patch. Loadable sections (e.g., .text, .data) may need to be aligned. Other sections generally don't need any alignment, but if they're aligned, the RawPointer should be adjusted before writing the section. Then a dwarf-specific function wouldn't be needed. Also when compiling in 64-bit mode, the assert can never be triggered, because MaxRawDataSize is UINT64_MAX.

[scott.linder]

LGTM, although I'm also fine with a more "functional" approach where RawPointer is returned rather than mutated via reference! I'll leave that between you and @stephenpeckham

Also, if you want to add a TODO for the cleanup @stephenpeckham suggests feel free to do so when landing! I'm at least happy that the function is now virtual, rather than just aliased in a subclass.

[Jake-Egan]

-Return the new RawPointer
-Added a TODO

[Jake-Egan]

rerun CI

llvm/lib/MC/XCOFFObjectWriter.cpp

struct SectionEntry {
   uint64_t Size;
   uint64_t FileOffsetToData;
   uint64_t FileOffsetToRelocations;
   uint32_t RelocationCount;
   int32_t Flags;
 
   int16_t Index;
 
+  virtual uint64_t advanceFileOffset(const uint64_t MaxRawDataSize,
+                                     const uint64_t RawPointer) {
+    FileOffsetToData = RawPointer;
+    uint64_t NewPointer = RawPointer + Size;
+    if (NewPointer > MaxRawDataSize)
+      report_fatal_error("Section raw data overflowed this object file.");
+    return NewPointer;
+  }
+
   // XCOFF has special section numbers for symbols:
   // -2 Specifies N_DEBUG, a special symbolic debugging symbol.
   // -1 Specifies N_ABS, an absolute symbol. The symbol has a value but is not
   // relocatable.
   //  0 Specifies N_UNDEF, an undefined external symbol.
   // Therefore, we choose -3 (N_DEBUG - 1) to represent a section index that
   // hasn't been initialized.
   static constexpr int16_t UninitializedIndex =
void reset() override {
     // Clear any csects we have stored.
     for (auto *Group : Groups)
       Group->clear();
   }
 
   virtual ~CsectSectionEntry() = default;
 };
 
 struct DwarfSectionEntry : public SectionEntry {
   // For DWARF section entry.
   std::unique_ptr<XCOFFSection> DwarfSect;
 
   // For DWARF section, we must use real size in the section header. MemorySize
   // is for the size the DWARF section occupies including paddings.
   uint32_t MemorySize;
 
+  // TODO: Remove this override. Loadable sections (e.g., .text, .data) may need
+  // to be aligned. Other sections generally don't need any alignment, but if
+  // they're aligned, the RawPointer should be adjusted before writing the
+  // section. Then a dwarf-specific function wouldn't be needed.
+  uint64_t advanceFileOffset(const uint64_t MaxRawDataSize,
+                             const uint64_t RawPointer) override {
+    FileOffsetToData = RawPointer;
+    uint64_t NewPointer = RawPointer + MemorySize;
+    assert(NewPointer <= MaxRawDataSize &&
+           "Section raw data overflowed this object file.");
+    return NewPointer;
+  }
+
   DwarfSectionEntry(StringRef N, int32_t Flags,
                     std::unique_ptr<XCOFFSection> Sect)
       : SectionEntry(N, Flags | XCOFF::STYP_DWARF), DwarfSect(std::move(Sect)),
         MemorySize(0) {
     assert(DwarfSect->MCSec->isDwarfSect() &&
            "This should be a DWARF section!");
     assert(N.size() <= XCOFF::NameSize && "section name too long");
     memcpy(Name, N.data(), N.size());
uint64_t RawPointer =
                     SectionCount * XCOFF::SectionHeaderSize32)) +
       auxiliaryHeaderSize();
 
   // Calculate the file offset to the section data.
   for (auto *Sec : Sections) {
     if (Sec->Index == SectionEntry::UninitializedIndex || Sec->IsVirtual)
       continue;
 
-    Sec->FileOffsetToData = RawPointer;
-    RawPointer += Sec->Size;
-    if (RawPointer > MaxRawDataSize)
-      report_fatal_error("Section raw data overflowed this object file.");
+    RawPointer = Sec->advanceFileOffset(MaxRawDataSize, RawPointer);
   }
 
   if (!DwarfSections.empty()) {
     RawPointer += PaddingsBeforeDwarf;
     for (auto &DwarfSection : DwarfSections) {
-      DwarfSection.FileOffsetToData = RawPointer;
-      RawPointer += DwarfSection.MemorySize;
-      if (RawPointer > MaxRawDataSize)
-        report_fatal_error("Section raw data overflowed this object file.");
+      RawPointer = DwarfSection.advanceFileOffset(MaxRawDataSize, RawPointer);
     }
   }
 
   if (hasExceptionSection()) {
-    ExceptionSection.FileOffsetToData = RawPointer;
-    RawPointer += ExceptionSection.Size;
-
-    assert(RawPointer <= MaxRawDataSize &&
-           "Section raw data overflowed this object file.");
+    RawPointer = ExceptionSection.advanceFileOffset(MaxRawDataSize, RawPointer);
   }
 
   for (auto *Sec : Sections) {
     if (Sec->Index != SectionEntry::UninitializedIndex)
       calcOffsetToRelocations(Sec, RawPointer);
   }
 
   for (auto &DwarfSec : DwarfSections)