[NFC][XCOFF][AIX] Serialize object file writing for each CsectGroup

[jasonliu]

Summary

Right now we handle each CsectGroup(ProgramCodeCsects, BSSCsects) individually when assigning indices, writing symbol table, and writing section raw data. However, there is already a pattern there, and we could common up those actions for every CsectGroup. This will make adding new CsectGroup(Read Write data, Read only data, TC/TOC, mergeable string) easier, and less error prone.

[daltenty]

llvm/lib/MC/XCOFFObjectWriter.cpp

114

nit: have -> has

262

nit: this unreachable is unneeded because of the default case above

[daltenty]

llvm/lib/MC/XCOFFObjectWriter.cpp

121

Should we specify this relative to XCOFF::N_DEBUG?

[jasonliu]

Address David's comments.

[sfertile]

llvm/lib/MC/XCOFFObjectWriter.cpp

83

Minor nit:
We have a 2 stage problem when deciding to use label defs or not:
Global: Does the type of csect allow/support labels defs.
Local: If label defs are supported in the csect type, can we omit the label defs for this specific csect.

I believe right now we are always emitting a label def for csects that support them even if it could be skipped, however there has been discussion to skip them when appropriate. UseLabelDefinition to me sounds like its saying implying yes to both the global and local criteria. In that light maybe enum values of 'LabelDefSupported' and 'LabelDefUnsupported' are more appropriate. (Or we can adjust the names when we implement the selective label def emission).

166–168

Minor nit: IIUC this is all the sections, as opposed to the non-empty sections. We can determine if a section is non-empty when it has been assigned a valid section index.

329

Should we maintain CurrentAddressLocation across groups to make sure there is no padding between groups?

573

I think we should consider using llvm::all_of to check if all the groups are empty and continue if they are. Then the section initialization can be done before processing any of the csects, without having to gate the initialization on if its section index is already valid , as well as adjust the size after processing the csects without an addition check to see if the section was initialized or not.

586

I think a fatal error would be more appropriate.

Minor nit: should we be creating a named constant for readability/documentation.

[jasonliu]

Address Sean's comment.

llvm/lib/MC/XCOFFObjectWriter.cpp

83

Agree.

329

Do you mean maintain CurrentAddressLocation across Sections? Since it's currently already maintained across groups.
Made the changes based on my interpretation.

573

I still need a guard for
Section->Address = Group->Csects.front().Address;

But I do think using a specific guard for that is better than just comparing UninitializedIndex everywhere.
Thanks.

[DiggerLin]

llvm/lib/MC/XCOFFObjectWriter.cpp

121

in the symbol table , the section index maybe has -2 ,-1 ,and 0. but for section, it always start from 1, I think keep original -1 as uninitiated is OK too.

573

two Csect.Size = Layout.getSectionAddressSize(MCSec); (duplicate with line 568 ) ?

[jasonliu]

llvm/lib/MC/XCOFFObjectWriter.cpp

121

It might be Okay depending on how we write our code later. But there is a chance we could run into risks of needing differentiate a special section number with a truely uninitialized section. So I would prefer to pick a section number that's not used to represent other things.

573

Thanks. I will address.

[jasonliu]

Address Digger's comment.

[jasonliu]

@DiggerLin @daltenty @sfertile
Ping.
I believe I have addressed the comments you put.

[daltenty]

LGTM

[Yi-Hong.Lyu]

Seems this commit broke the buildbot http://lab.llvm.org:8011/builders/ppc64le-lld-multistage-test/builds/6795

llvm/lib/MC/XCOFFObjectWriter.cpp

 // explicitly initialized or not.
 //
 // We don't represent the sections in the MC layer as there is nothing
 // interesting about them at at that level: they carry information that is
 // only relevant to the ObjectWriter, so we materialize them in this class.
 namespace {
 
 constexpr unsigned DefaultSectionAlign = 4;
+constexpr int16_t MaxSectionIndex = INT16_MAX;
 
 // Packs the csect's alignment and type into a byte.
 uint8_t getEncodedType(const MCSectionXCOFF *);
 
 // Wrapper around an MCSymbolXCOFF.
 struct Symbol {
   const MCSymbolXCOFF *const MCSym;
   uint32_t SymbolTableIndex;
struct ControlSection {
   uint32_t Size;
 
   SmallVector<Symbol, 1> Syms;
   StringRef getName() const { return MCCsect->getSectionName(); }
   ControlSection(const MCSectionXCOFF *MCSec)
       : MCCsect(MCSec), SymbolTableIndex(-1), Address(-1), Size(0) {}
 };
 
+// Type to be used for a container representing a set of csects with
+// (approximately) the same storage mapping class. For example all the csects
+// with a storage mapping class of `xmc_pr` will get placed into the same
+// container.
+struct CsectGroup {
+  enum LabelDefinitionSupport : bool {
+    LabelDefSupported = true,
+    LabelDefUnsupported = false
+  };
+
+  const LabelDefinitionSupport SupportLabelDef;
+  std::deque<ControlSection> Csects;
+};
+
+using CsectGroups = std::deque<CsectGroup *>;
+
 // Represents the data related to a section excluding the csects that make up
 // the raw data of the section. The csects are stored separately as not all
 // sections contain csects, and some sections contain csects which are better
 // stored separately, e.g. the .data section containing read-write, descriptor,
 // TOCBase and TOC-entry csects.
 struct Section {
   char Name[XCOFF::NameSize];
   // The physical/virtual address of the section. For an object file
   // these values are equivalent.
   uint32_t Address;
   uint32_t Size;
   uint32_t FileOffsetToData;
   uint32_t FileOffsetToRelocations;
   uint32_t RelocationCount;
   int32_t Flags;
 
   int16_t Index;
 
   // Virtual sections do not need storage allocated in the object file.
   const bool IsVirtual;
 
+  // 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 =
+      XCOFF::ReservedSectionNum::N_DEBUG - 1;
+
+  CsectGroups Groups;
+
   void reset() {
     Address = 0;
     Size = 0;
     FileOffsetToData = 0;
     FileOffsetToRelocations = 0;
     RelocationCount = 0;
-    Index = -1;
+    Index = UninitializedIndex;
+    // Clear any csects we have stored.
+    for (auto *Group : Groups)
+      Group->Csects.clear();
   }
 
-  Section(const char *N, XCOFF::SectionTypeFlags Flags, bool IsVirtual)
+  Section(const char *N, XCOFF::SectionTypeFlags Flags, bool IsVirtual,
+          CsectGroups Groups)
       : Address(0), Size(0), FileOffsetToData(0), FileOffsetToRelocations(0),
-        RelocationCount(0), Flags(Flags), Index(-1), IsVirtual(IsVirtual) {
+        RelocationCount(0), Flags(Flags), Index(UninitializedIndex),
+        IsVirtual(IsVirtual), Groups(Groups) {
     strncpy(Name, N, XCOFF::NameSize);
   }
 };
 
 class XCOFFObjectWriter : public MCObjectWriter {
-  // Type to be used for a container representing a set of csects with
-  // (approximately) the same storage mapping class. For example all the csects
-  // with a storage mapping class of `xmc_pr` will get placed into the same
-  // container.
-  using CsectGroup = std::deque<ControlSection>;
+
+  uint32_t SymbolTableEntryCount = 0;
+  uint32_t SymbolTableOffset = 0;
 
   support::endian::Writer W;
   std::unique_ptr<MCXCOFFObjectTargetWriter> TargetObjectWriter;
   StringTableBuilder Strings;
 
-  // The non-empty sections, in the order they will appear in the section header
-  // table.
-  std::vector<Section *> Sections;
+  // CsectGroups. These store the csects which make up different parts of
+  // the sections. Should have one for each set of csects that get mapped into
+  // the same section and get handled in a 'similar' way.
+  CsectGroup ProgramCodeCsects{CsectGroup::LabelDefSupported};
+  CsectGroup BSSCsects{CsectGroup::LabelDefUnsupported};
 
   // The Predefined sections.
   Section Text;
   Section BSS;
 
-  // CsectGroups. These store the csects which make up different parts of
-  // the sections. Should have one for each set of csects that get mapped into
-  // the same section and get handled in a 'similar' way.
-  CsectGroup ProgramCodeCsects;
-  CsectGroup BSSCsects;
+  // All the XCOFF sections, in the order they will appear in the section header
+  // table.
+  std::array<Section *const, 2> Sections{&Text, &BSS};
 
-  uint32_t SymbolTableEntryCount = 0;
-  uint32_t SymbolTableOffset = 0;
+  CsectGroup &getCsectGroup(const MCSectionXCOFF *MCSec);
 
   virtual void reset() override;
 
   void executePostLayoutBinding(MCAssembler &, const MCAsmLayout &) override;
 
   void recordRelocation(MCAssembler &, const MCAsmLayout &, const MCFragment *,
                         const MCFixup &, MCValue, uint64_t &) override;
 
public:
   XCOFFObjectWriter(std::unique_ptr<MCXCOFFObjectTargetWriter> MOTW,
                     raw_pwrite_stream &OS);
 };
 
 XCOFFObjectWriter::XCOFFObjectWriter(
     std::unique_ptr<MCXCOFFObjectTargetWriter> MOTW, raw_pwrite_stream &OS)
     : W(OS, support::big), TargetObjectWriter(std::move(MOTW)),
       Strings(StringTableBuilder::XCOFF),
-      Text(".text", XCOFF::STYP_TEXT, /* IsVirtual */ false),
-      BSS(".bss", XCOFF::STYP_BSS, /* IsVirtual */ true) {}
+      Text(".text", XCOFF::STYP_TEXT, /* IsVirtual */ false,
+           CsectGroups{&ProgramCodeCsects}),
+      BSS(".bss", XCOFF::STYP_BSS, /* IsVirtual */ true,
+          CsectGroups{&BSSCsects}) {}
 
 void XCOFFObjectWriter::reset() {
   // Reset any sections we have written to, and empty the section header table.
   for (auto *Sec : Sections)
     Sec->reset();
-  Sections.clear();
-
-  // Clear any csects we have stored.
-  ProgramCodeCsects.clear();
-  BSSCsects.clear();
 
   // Reset the symbol table and string table.
   SymbolTableEntryCount = 0;
   SymbolTableOffset = 0;
   Strings.clear();
 
   MCObjectWriter::reset();
 }
 
+CsectGroup &XCOFFObjectWriter::getCsectGroup(const MCSectionXCOFF *MCSec) {
+  switch (MCSec->getMappingClass()) {
+  case XCOFF::XMC_PR:
+    assert(XCOFF::XTY_SD == MCSec->getCSectType() &&
+           "Only an initialized csect can contain program code.");
+    return ProgramCodeCsects;
+  case XCOFF::XMC_RW:
+    if (XCOFF::XTY_CM == MCSec->getCSectType())
+      return BSSCsects;
+
+    report_fatal_error("Unhandled mapping of read-write csect to section.");
+  case XCOFF::XMC_BS:
+    assert(XCOFF::XTY_CM == MCSec->getCSectType() &&
+           "Mapping invalid csect. CSECT with bss storage class must be "
+           "common type.");
+    return BSSCsects;
+  default:
+    report_fatal_error("Unhandled mapping of csect to section.");
+  }
+}
+
 void XCOFFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
                                                  const MCAsmLayout &Layout) {
   if (TargetObjectWriter->is64Bit())
     report_fatal_error("64-bit XCOFF object files are not supported yet.");
 
   // Maps the MC Section representation to its corresponding ControlSection
   // wrapper. Needed for finding the ControlSection to insert an MCSymbol into
   // from its containing MCSectionXCOFF.
   DenseMap<const MCSectionXCOFF *, ControlSection *> WrapperMap;
 
   for (const auto &S : Asm) {
     const auto *MCSec = cast<const MCSectionXCOFF>(&S);
     assert(WrapperMap.find(MCSec) == WrapperMap.end() &&
            "Cannot add a csect twice.");
 
     // If the name does not fit in the storage provided in the symbol table
     // entry, add it to the string table.
     if (nameShouldBeInStringTable(MCSec->getSectionName()))
       Strings.add(MCSec->getSectionName());
 
-    switch (MCSec->getMappingClass()) {
-    case XCOFF::XMC_PR:
-      assert(XCOFF::XTY_SD == MCSec->getCSectType() &&
-             "Only an initialized csect can contain program code.");
-      ProgramCodeCsects.emplace_back(MCSec);
-      WrapperMap[MCSec] = &ProgramCodeCsects.back();
-      break;
-    case XCOFF::XMC_RW:
-      if (XCOFF::XTY_CM == MCSec->getCSectType()) {
-        BSSCsects.emplace_back(MCSec);
-        WrapperMap[MCSec] = &BSSCsects.back();
-        break;
-      }
-      report_fatal_error("Unhandled mapping of read-write csect to section.");
-    case XCOFF::XMC_TC0:
-      // TODO FIXME Handle emiting the TOC base.
-      break;
-    case XCOFF::XMC_BS:
-      assert(XCOFF::XTY_CM == MCSec->getCSectType() &&
-             "Mapping invalid csect. CSECT with bss storage class must be "
-             "common type.");
-      BSSCsects.emplace_back(MCSec);
-      WrapperMap[MCSec] = &BSSCsects.back();
-      break;
-    default:
-      report_fatal_error("Unhandled mapping of csect to section.");
-    }
+    if (MCSec->getMappingClass() == XCOFF::XMC_TC0)
+      continue;
+
+    CsectGroup &Group = getCsectGroup(MCSec);
+    Group.Csects.emplace_back(MCSec);
+    WrapperMap[MCSec] = &Group.Csects.back();
   }
 
   for (const MCSymbol &S : Asm.symbols()) {
     // Nothing to do for temporary symbols.
     if (S.isTemporary())
       continue;
     const MCSymbolXCOFF *XSym = cast<MCSymbolXCOFF>(&S);
 

 void XCOFFObjectWriter::recordRelocation(MCAssembler &, const MCAsmLayout &,
                                          const MCFragment *, const MCFixup &,
                                          MCValue, uint64_t &) {
   report_fatal_error("XCOFF relocations not supported.");
 }
 
 void XCOFFObjectWriter::writeSections(const MCAssembler &Asm,
                                       const MCAsmLayout &Layout) {
-  // Write the program code control sections one at a time.
-  uint32_t CurrentAddressLocation = Text.Address;
-  for (const auto &Csect : ProgramCodeCsects) {
+  uint32_t CurrentAddressLocation = 0;
+  for (const auto *Section : Sections) {
+    // Nothing to write for this Section.
+    if (Section->Index == Section::UninitializedIndex || Section->IsVirtual)
+      continue;
+
+    assert(CurrentAddressLocation == Section->Address &&
+           "We should have no padding between sections.");
+    for (const auto *Group : Section->Groups) {
+      for (const auto &Csect : Group->Csects) {
         if (uint32_t PaddingSize = Csect.Address - CurrentAddressLocation)
           W.OS.write_zeros(PaddingSize);
         Asm.writeSectionData(W.OS, Csect.MCCsect, Layout);
         CurrentAddressLocation = Csect.Address + Csect.Size;
       }
+    }
 
-  if (Text.Index != -1) {
     // The size of the tail padding in a section is the end virtual address of
     // the current section minus the the end virtual address of the last csect
     // in that section.
     if (uint32_t PaddingSize =
-            Text.Address + Text.Size - CurrentAddressLocation)
+            Section->Address + Section->Size - CurrentAddressLocation)
       W.OS.write_zeros(PaddingSize);
   }
 }
 
 uint64_t XCOFFObjectWriter::writeObject(MCAssembler &Asm,
                                         const MCAsmLayout &Layout) {
   // We always emit a timestamp of 0 for reproducibility, so ensure incremental
   // linking is not enabled, in case, like with Windows COFF, such a timestamp
for (const auto *Sec : Sections) {
     W.write<uint16_t>(0);
     W.write<uint16_t>(0);
 
     W.write<int32_t>(Sec->Flags);
   }
 }
 
 void XCOFFObjectWriter::writeSymbolTable(const MCAsmLayout &Layout) {
-  // Print out symbol table for the program code.
-  for (const auto &Csect : ProgramCodeCsects) {
+  for (const auto *Section : Sections) {
+    for (const auto *Group : Section->Groups) {
+      if (Group->Csects.empty())
+        continue;
+
+      const bool SupportLabelDef = Group->SupportLabelDef;
+      const int16_t SectionIndex = Section->Index;
+      for (const auto &Csect : Group->Csects) {
         // Write out the control section first and then each symbol in it.
-    writeSymbolTableEntryForControlSection(Csect, Text.Index,
-                                           Csect.MCCsect->getStorageClass());
-    for (const auto &Sym : Csect.Syms)
-      writeSymbolTableEntryForCsectMemberLabel(
-          Sym, Csect, Text.Index, Layout.getSymbolOffset(*Sym.MCSym));
+        writeSymbolTableEntryForControlSection(
+            Csect, SectionIndex, Csect.MCCsect->getStorageClass());
+        if (!SupportLabelDef) {
+          assert(Csect.Syms.size() == 1 && "Csect should only contain 1 symbol "
+                                           "which is its label definition.");
+          continue;
         }
 
-  // The BSS Section is special in that the csects must contain a single symbol,
-  // and the contained symbol cannot be represented in the symbol table as a
-  // label definition.
-  for (auto &Csect : BSSCsects) {
-    assert(Csect.Syms.size() == 1 &&
-           "Uninitialized csect cannot contain more then 1 symbol.");
-    Symbol &Sym = Csect.Syms.back();
-    writeSymbolTableEntryForControlSection(Csect, BSS.Index,
-                                           Sym.getStorageClass());
+        for (const auto Sym : Csect.Syms)
+          writeSymbolTableEntryForCsectMemberLabel(
+              Sym, Csect, SectionIndex, Layout.getSymbolOffset(*(Sym.MCSym)));
+      }
+    }
   }
 }
 
 void XCOFFObjectWriter::assignAddressesAndIndices(const MCAsmLayout &Layout) {
   // The address corrresponds to the address of sections and symbols in the
   // object file. We place the shared address 0 immediately after the
   // section header table.
   uint32_t Address = 0;
   // Section indices are 1-based in XCOFF.
-  int16_t SectionIndex = 1;
+  int32_t SectionIndex = 1;
   // The first symbol table entry is for the file name. We are not emitting it
   // yet, so start at index 0.
   uint32_t SymbolTableIndex = 0;
 
-  // Text section comes first.
-  if (!ProgramCodeCsects.empty()) {
-    Sections.push_back(&Text);
-    Text.Index = SectionIndex++;
-    for (auto &Csect : ProgramCodeCsects) {
+  for (auto *Section : Sections) {
+    const bool IsEmpty =
+        llvm::all_of(Section->Groups, [](const CsectGroup *Group) {
+          return Group->Csects.empty();
+        });
+    if (IsEmpty)
+      continue;
+
+    if (SectionIndex > MaxSectionIndex)
+      report_fatal_error("Section index overflow!");
+    Section->Index = SectionIndex++;
+
+    bool SectionAddressSet = false;
+    for (auto *Group : Section->Groups) {
+      if (Group->Csects.empty())
+        continue;
+
+      const bool SupportLabelDef = Group->SupportLabelDef;
+      for (auto &Csect : Group->Csects) {
         const MCSectionXCOFF *MCSec = Csect.MCCsect;
         Csect.Address = alignTo(Address, MCSec->getAlignment());
         Csect.Size = Layout.getSectionAddressSize(MCSec);
         Address = Csect.Address + Csect.Size;
         Csect.SymbolTableIndex = SymbolTableIndex;
         // 1 main and 1 auxiliary symbol table entry for the csect.
         SymbolTableIndex += 2;
+
+        if (!SupportLabelDef)
+          continue;
+
         for (auto &Sym : Csect.Syms) {
           Sym.SymbolTableIndex = SymbolTableIndex;
-        // 1 main and 1 auxiliary symbol table entry for each contained symbol
+          // 1 main and 1 auxiliary symbol table entry for each contained
+          // symbol.
           SymbolTableIndex += 2;
         }
       }
-    Address = alignTo(Address, DefaultSectionAlign);
-
-    // The first csect of a section can be aligned by adjusting the virtual
-    // address of its containing section instead of writing zeroes into the
-    // object file.
-    Text.Address = ProgramCodeCsects.front().Address;
 
-    Text.Size = Address - Text.Address;
+      if (!SectionAddressSet) {
+        Section->Address = Group->Csects.front().Address;
+        SectionAddressSet = true;
       }
-
-  // Data section Second. TODO
-
-  // BSS Section third.
-  if (!BSSCsects.empty()) {
-    Sections.push_back(&BSS);
-    BSS.Index = SectionIndex++;
-    for (auto &Csect : BSSCsects) {
-      const MCSectionXCOFF *MCSec = Csect.MCCsect;
-      Csect.Address = alignTo(Address, MCSec->getAlignment());
-      Csect.Size = Layout.getSectionAddressSize(MCSec);
-      Address = Csect.Address + Csect.Size;
-      Csect.SymbolTableIndex = SymbolTableIndex;
-      // 1 main and 1 auxiliary symbol table entry for the csect.
-      SymbolTableIndex += 2;
-
-      assert(Csect.Syms.size() == 1 &&
-             "csect in the BSS can only contain a single symbol.");
-      Csect.Syms[0].SymbolTableIndex = Csect.SymbolTableIndex;
     }
-    // Pad out Address to the default alignment. This is to match how the system
-    // assembler handles the .bss section. Its size is always a multiple of 4.
-    Address = alignTo(Address, DefaultSectionAlign);
 
-    BSS.Address = BSSCsects.front().Address;
-    BSS.Size = Address - BSS.Address;
+    // Make sure the address of the next section aligned to
+    // DefaultSectionAlign.
+    Address = alignTo(Address, DefaultSectionAlign);
+    Section->Size = Address - Section->Address;
   }
 
   SymbolTableEntryCount = SymbolTableIndex;
 
   // Calculate the RawPointer value for each section.
   uint64_t RawPointer = sizeof(XCOFF::FileHeader32) + auxiliaryHeaderSize() +
                         Sections.size() * sizeof(XCOFF::SectionHeader32);
   for (auto *Sec : Sections) {