[XCOFF] [NFC] make csect properties optional for function getXCOFFSection

[shchenz]

Summary

This is from the review of D95518.
Per @hubert.reinterpretcast comments, we should make csect properties optional as they are not used by newly introduced debug sections in D95518.

[shchenz]

gentle ping

[hubert.reinterpretcast]

llvm/include/llvm/BinaryFormat/XCOFF.h

409

There's no need to use the C-style typedef + tag type pattern in C++ code. Also, the name should reflect that this type stores csect properties (and is not a selector between csect properties).

410

Because we're using llvm::Optional, we can use the enumeration types.

Moot point: Is attaching "V" in front a common pattern in LLVM? The way the name lookup works means that using the same name for the constructor parameter and the class member is fine.

411–414

I suggest keeping the very short names for block-scope variables only.

llvm/include/llvm/MC/MCContext.h

582–583

Use llvm::Optional.

[hubert.reinterpretcast]

llvm/lib/MC/MCContext.cpp

679

This is eventually going to need to handle the case where the section is not a csect, but for this patch, I think it is okay to write it as it all XCOFF sections are csects. The patch that introduces non-csect XCOFF sections will need to do the update to this.

[shchenz]

1: use llvm::Optional

[shchenz]

@hubert.reinterpretcast Thanks for your comments. Updated accordingly.

llvm/include/llvm/BinaryFormat/XCOFF.h

410

Very good idea. Before using Optional, I have to use 0xff as the default value for StorageMappingClass and SymbolType, so I need a unsigned type for the parameter. Now I can use the enumeration type because the default value for an optional parameter can be None.

Is attaching "V" in front a common pattern in LLVM

Seems this is not common use. I added that for differentiating the class member name and function parameter name. But you are right, we are ok here to use the same name for them.

llvm/lib/MC/MCContext.cpp

679

Updated in D95518 and I added a new NFC patch D96641 to make class member also optional

[hubert.reinterpretcast]

LGTM with minor comment.

llvm/lib/MC/MCContext.cpp

679

Thanks. Let's have a TODO comment about this function's need for revision to handle non-csects in the code anyway (so this patch is better self-contained).

[shchenz]

1: add the FIXME for None CsectProp

llvm/include/llvm/BinaryFormat/XCOFF.h

enum ExtendedTBTableFlag : uint8_t {
   TB_OS2 = 0x10,         ///< Reserved for OS use.
   TB_EH_INFO = 0x08,     ///< Exception handling info present.
   TB_LONGTBTABLE2 = 0x01 ///< Additional tbtable extension exists.
 };
 
 StringRef getNameForTracebackTableLanguageId(TracebackTable::LanguageID LangId);
 SmallString<32> getExtendedTBTableFlagString(uint8_t Flag);
 
+struct CsectProperties {
+  CsectProperties(StorageMappingClass SMC, SymbolType ST)
+      : MappingClass(SMC), Type(ST) {}
+  StorageMappingClass MappingClass;
+  SymbolType Type;
+};
+
 } // end namespace XCOFF
 } // end namespace llvm
 
 #endif

llvm/include/llvm/MC/MCContext.h

public:
     MCSectionWasm *getWasmSection(const Twine &Section, SectionKind K,
                                   const Twine &Group, unsigned UniqueID,
                                   const char *BeginSymName);
 
     MCSectionWasm *getWasmSection(const Twine &Section, SectionKind K,
                                   const MCSymbolWasm *Group, unsigned UniqueID,
                                   const char *BeginSymName);
 
-    MCSectionXCOFF *getXCOFFSection(StringRef Section,
-                                    XCOFF::StorageMappingClass MappingClass,
-                                    XCOFF::SymbolType CSectType, SectionKind K,
-                                    bool MultiSymbolsAllowed = false,
-                                    const char *BeginSymName = nullptr);
+    MCSectionXCOFF *
+    getXCOFFSection(StringRef Section, SectionKind K,
+                    Optional<XCOFF::CsectProperties> CsectProp = None,
 
     // Create and save a copy of STI and return a reference to the copy.
     MCSubtargetInfo &getSubtargetCopy(const MCSubtargetInfo &STI);
 
     /// @}
 
     /// \name Dwarf Management
     /// @{

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

if (Kind.isText())
     MappingClass = XCOFF::XMC_PR;
   else if (Kind.isData() || Kind.isReadOnlyWithRel() || Kind.isBSS())
     MappingClass = XCOFF::XMC_RW;
   else if (Kind.isReadOnly())
     MappingClass = XCOFF::XMC_RO;
   else
     report_fatal_error("XCOFF other section types not yet implemented.");
 
-  return getContext().getXCOFFSection(SectionName, MappingClass, XCOFF::XTY_SD,
-                                      Kind, /* MultiSymbolsAllowed*/ true);
+  return getContext().getXCOFFSection(
+      SectionName, Kind, XCOFF::CsectProperties(MappingClass, XCOFF::XTY_SD),
+      /* MultiSymbolsAllowed*/ true);
 }
 
 MCSection *TargetLoweringObjectFileXCOFF::getSectionForExternalReference(
     const GlobalObject *GO, const TargetMachine &TM) const {
   assert(GO->isDeclarationForLinker() &&
          "Tried to get ER section for a defined global.");
 
   SmallString<128> Name;
   getNameWithPrefix(Name, GO, TM);
 
   // Externals go into a csect of type ER.
   return getContext().getXCOFFSection(
-      Name, isa<Function>(GO) ? XCOFF::XMC_DS : XCOFF::XMC_UA, XCOFF::XTY_ER,
-      SectionKind::getMetadata());
+      Name, SectionKind::getMetadata(),
+      XCOFF::CsectProperties(isa<Function>(GO) ? XCOFF::XMC_DS : XCOFF::XMC_UA,
+                             XCOFF::XTY_ER));
 }
 
 MCSection *TargetLoweringObjectFileXCOFF::SelectSectionForGlobal(
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
   // Common symbols go into a csect with matching name which will get mapped
   // into the .bss section.
   if (Kind.isBSSLocal() || Kind.isCommon()) {
     SmallString<128> Name;
     getNameWithPrefix(Name, GO, TM);
     return getContext().getXCOFFSection(
-        Name, Kind.isBSSLocal() ? XCOFF::XMC_BS : XCOFF::XMC_RW, XCOFF::XTY_CM,
-        Kind);
+        Name, Kind,
+        XCOFF::CsectProperties(
+            Kind.isBSSLocal() ? XCOFF::XMC_BS : XCOFF::XMC_RW, XCOFF::XTY_CM));
   }
 
   if (Kind.isMergeableCString()) {
     Align Alignment = GO->getParent()->getDataLayout().getPreferredAlign(
         cast<GlobalVariable>(GO));
 
     unsigned EntrySize = getEntrySizeForKind(Kind);
     std::string SizeSpec = ".rodata.str" + utostr(EntrySize) + ".";
     SmallString<128> Name;
     Name = SizeSpec + utostr(Alignment.value());
 
     if (TM.getDataSections())
       getNameWithPrefix(Name, GO, TM);
 
     return getContext().getXCOFFSection(
-        Name, XCOFF::XMC_RO, XCOFF::XTY_SD, Kind,
+        Name, Kind, XCOFF::CsectProperties(XCOFF::XMC_RO, XCOFF::XTY_SD),
         /* MultiSymbolsAllowed*/ !TM.getDataSections());
   }
 
   if (Kind.isText()) {
     if (TM.getFunctionSections()) {
       return cast<MCSymbolXCOFF>(getFunctionEntryPointSymbol(GO, TM))
           ->getRepresentedCsect();
     }
     return TextSection;
   }
 
   // TODO: We may put Kind.isReadOnlyWithRel() under option control, because
   // user may want to have read-only data with relocations placed into a
   // read-only section by the compiler.
   // For BSS kind, zero initialized data must be emitted to the .data section
   // because external linkage control sections that get mapped to the .bss
   // section will be linked as tentative defintions, which is only appropriate
   // for SectionKind::Common.
   if (Kind.isData() || Kind.isReadOnlyWithRel() || Kind.isBSS()) {
     if (TM.getDataSections()) {
       SmallString<128> Name;
       getNameWithPrefix(Name, GO, TM);
-      return getContext().getXCOFFSection(Name, XCOFF::XMC_RW, XCOFF::XTY_SD,
-                                          SectionKind::getData());
+      return getContext().getXCOFFSection(
+          Name, SectionKind::getData(),
+          XCOFF::CsectProperties(XCOFF::XMC_RW, XCOFF::XTY_SD));
     }
     return DataSection;
   }
 
   if (Kind.isReadOnly()) {
     if (TM.getDataSections()) {
       SmallString<128> Name;
       getNameWithPrefix(Name, GO, TM);
-      return getContext().getXCOFFSection(Name, XCOFF::XMC_RO, XCOFF::XTY_SD,
-                                          SectionKind::getReadOnly());
+      return getContext().getXCOFFSection(
+          Name, SectionKind::getReadOnly(),
+          XCOFF::CsectProperties(XCOFF::XMC_RO, XCOFF::XTY_SD));
     }
     return ReadOnlySection;
   }
 
   report_fatal_error("XCOFF other section types not yet implemented.");
 }
 
 MCSection *TargetLoweringObjectFileXCOFF::getSectionForJumpTable(
     const Function &F, const TargetMachine &TM) const {
   assert (!F.getComdat() && "Comdat not supported on XCOFF.");
 
   if (!TM.getFunctionSections())
     return ReadOnlySection;
 
   // If the function can be removed, produce a unique section so that
   // the table doesn't prevent the removal.
   SmallString<128> NameStr(".rodata.jmp..");
   getNameWithPrefix(NameStr, &F, TM);
-  return getContext().getXCOFFSection(NameStr, XCOFF::XMC_RO, XCOFF::XTY_SD,
-                                      SectionKind::getReadOnly());
+  return getContext().getXCOFFSection(
+      NameStr, SectionKind::getReadOnly(),
+      XCOFF::CsectProperties(XCOFF::XMC_RO, XCOFF::XTY_SD));
 }
 
 bool TargetLoweringObjectFileXCOFF::shouldPutJumpTableInFunctionSection(
     bool UsesLabelDifference, const Function &F) const {
   return false;
 }
 
 /// Given a mergeable constant with the specified size and relocation
MCSymbol *TargetLoweringObjectFileXCOFF::getFunctionEntryPointSymbol(
   // When -function-sections is enabled and explicit section is not specified,
   // it's not necessary to emit function entry point label any more. We will use
   // function entry point csect instead. And for function delcarations, the
   // undefined symbols gets treated as csect with XTY_ER property.
   if (((TM.getFunctionSections() && !Func->hasSection()) ||
        Func->isDeclaration()) &&
       isa<Function>(Func)) {
     return getContext()
-        .getXCOFFSection(NameStr, XCOFF::XMC_PR,
-                         Func->isDeclaration() ? XCOFF::XTY_ER : XCOFF::XTY_SD,
-                         SectionKind::getText())
+        .getXCOFFSection(
+            NameStr, SectionKind::getText(),
+            XCOFF::CsectProperties(XCOFF::XMC_PR, Func->isDeclaration()
+                                                      ? XCOFF::XTY_ER
+                                                      : XCOFF::XTY_SD))
         ->getQualNameSymbol();
   }
 
   return getContext().getOrCreateSymbol(NameStr);
 }
 
 MCSection *TargetLoweringObjectFileXCOFF::getSectionForFunctionDescriptor(
     const Function *F, const TargetMachine &TM) const {
   SmallString<128> NameStr;
   getNameWithPrefix(NameStr, F, TM);
-  return getContext().getXCOFFSection(NameStr, XCOFF::XMC_DS, XCOFF::XTY_SD,
-                                      SectionKind::getData());
+  return getContext().getXCOFFSection(
+      NameStr, SectionKind::getData(),
+      XCOFF::CsectProperties(XCOFF::XMC_DS, XCOFF::XTY_SD));
 }
 
 MCSection *TargetLoweringObjectFileXCOFF::getSectionForTOCEntry(
     const MCSymbol *Sym, const TargetMachine &TM) const {
   // Use TE storage-mapping class when large code model is enabled so that
   // the chance of needing -bbigtoc is decreased.
   return getContext().getXCOFFSection(
-      cast<MCSymbolXCOFF>(Sym)->getSymbolTableName(),
+      cast<MCSymbolXCOFF>(Sym)->getSymbolTableName(), SectionKind::getData(),
+      XCOFF::CsectProperties(
           TM.getCodeModel() == CodeModel::Large ? XCOFF::XMC_TE : XCOFF::XMC_TC,
-      XCOFF::XTY_SD, SectionKind::getData());
+          XCOFF::XTY_SD));
 }

llvm/lib/MC/MCContext.cpp

MCSectionWasm *MCContext::getWasmSection(const Twine &Section, SectionKind Kind,
   Result->getFragmentList().insert(Result->begin(), F);
   F->setParent(Result);
   Begin->setFragment(F);
 
   return Result;
 }
 
 MCSectionXCOFF *
-MCContext::getXCOFFSection(StringRef Section, XCOFF::StorageMappingClass SMC,
-                           XCOFF::SymbolType Type, SectionKind Kind,
+MCContext::getXCOFFSection(StringRef Section, SectionKind Kind,
+                           Optional<XCOFF::CsectProperties> CsectProp,
                            bool MultiSymbolsAllowed, const char *BeginSymName) {
   // Do the lookup. If we have a hit, return it.
-  auto IterBool = XCOFFUniquingMap.insert(
-      std::make_pair(XCOFFSectionKey{Section.str(), SMC}, nullptr));
+  // FIXME: handle the case for non-csect sections. Non-csect section has None
+  // CsectProp.
+  auto IterBool = XCOFFUniquingMap.insert(std::make_pair(
+      XCOFFSectionKey{Section.str(), CsectProp->MappingClass}, nullptr));
   auto &Entry = *IterBool.first;
   if (!IterBool.second) {
     MCSectionXCOFF *ExistedEntry = Entry.second;
     if (ExistedEntry->isMultiSymbolsAllowed() != MultiSymbolsAllowed)
       report_fatal_error("section's multiply symbols policy does not match");
 
     return ExistedEntry;
   }
 
   // Otherwise, return a new section.
   StringRef CachedName = Entry.first.SectionName;
   MCSymbolXCOFF *QualName = cast<MCSymbolXCOFF>(getOrCreateSymbol(
-      CachedName + "[" + XCOFF::getMappingClassString(SMC) + "]"));
+      CachedName + "[" + XCOFF::getMappingClassString(CsectProp->MappingClass) +
+      "]"));
 
   MCSymbol *Begin = nullptr;
   if (BeginSymName)
     Begin = createTempSymbol(BeginSymName, false);
 
   // QualName->getUnqualifiedName() and CachedName are the same except when
   // CachedName contains invalid character(s) such as '$' for an XCOFF symbol.
-  MCSectionXCOFF *Result = new (XCOFFAllocator.Allocate())
-      MCSectionXCOFF(QualName->getUnqualifiedName(), SMC, Type, Kind, QualName,
-                     Begin, CachedName, MultiSymbolsAllowed);
+  MCSectionXCOFF *Result = new (XCOFFAllocator.Allocate()) MCSectionXCOFF(
+      QualName->getUnqualifiedName(), CsectProp->MappingClass, CsectProp->Type,
+      Kind, QualName, Begin, CachedName, MultiSymbolsAllowed);
   Entry.second = Result;
 
   auto *F = new MCDataFragment();
   Result->getFragmentList().insert(Result->begin(), F);
   F->setParent(Result);
 
   if (Begin)
     Begin->setFragment(F);

llvm/lib/MC/MCObjectFileInfo.cpp

 }
 
 void MCObjectFileInfo::initXCOFFMCObjectFileInfo(const Triple &T) {
   // The default csect for program code. Functions without a specified section
   // get placed into this csect. The choice of csect name is not a property of
   // the ABI or object file format. For example, the XL compiler uses an unnamed
   // csect for program code.
   TextSection = Ctx->getXCOFFSection(
-      ".text", XCOFF::StorageMappingClass::XMC_PR, XCOFF::XTY_SD,
-      SectionKind::getText(), /* MultiSymbolsAllowed*/ true);
+      ".text", SectionKind::getText(),
+      XCOFF::CsectProperties(XCOFF::StorageMappingClass::XMC_PR, XCOFF::XTY_SD),
+      /* MultiSymbolsAllowed*/ true);
 
   DataSection = Ctx->getXCOFFSection(
-      ".data", XCOFF::StorageMappingClass::XMC_RW, XCOFF::XTY_SD,
-      SectionKind::getData(), /* MultiSymbolsAllowed*/ true);
+      ".data", SectionKind::getData(),
+      XCOFF::CsectProperties(XCOFF::StorageMappingClass::XMC_RW, XCOFF::XTY_SD),
+      /* MultiSymbolsAllowed*/ true);
 
   ReadOnlySection = Ctx->getXCOFFSection(
-      ".rodata", XCOFF::StorageMappingClass::XMC_RO, XCOFF::XTY_SD,
-      SectionKind::getReadOnly(), /* MultiSymbolsAllowed*/ true);
-
-  TOCBaseSection =
-      Ctx->getXCOFFSection("TOC", XCOFF::StorageMappingClass::XMC_TC0,
-                           XCOFF::XTY_SD, SectionKind::getData());
+      ".rodata", SectionKind::getReadOnly(),
+      XCOFF::CsectProperties(XCOFF::StorageMappingClass::XMC_RO, XCOFF::XTY_SD),
+      /* MultiSymbolsAllowed*/ true);
+
+  TOCBaseSection = Ctx->getXCOFFSection(
+      "TOC", SectionKind::getData(),
+      XCOFF::CsectProperties(XCOFF::StorageMappingClass::XMC_TC0,
+                             XCOFF::XTY_SD));
 
   // The TOC-base always has 0 size, but 4 byte alignment.
   TOCBaseSection->setAlignment(Align(4));
 
-  LSDASection = Ctx->getXCOFFSection(".gcc_except_table",
-                                     XCOFF::StorageMappingClass::XMC_RO,
-                                     XCOFF::XTY_SD, SectionKind::getReadOnly());
-
-  CompactUnwindSection =
-      Ctx->getXCOFFSection(".eh_info_table", XCOFF::StorageMappingClass::XMC_RW,
-                           XCOFF::XTY_SD, SectionKind::getData());
+  LSDASection = Ctx->getXCOFFSection(
+      ".gcc_except_table", SectionKind::getReadOnly(),
+      XCOFF::CsectProperties(XCOFF::StorageMappingClass::XMC_RO,
+                             XCOFF::XTY_SD));
+
+  CompactUnwindSection = Ctx->getXCOFFSection(
+      ".eh_info_table", SectionKind::getData(),
+      XCOFF::CsectProperties(XCOFF::StorageMappingClass::XMC_RW,
+                             XCOFF::XTY_SD));
 
   // DWARF sections for XCOFF are not csects. They are special STYP_DWARF
   // sections, and the individual DWARF sections are distinguished by their
   // section subtype.
   // TODO: Populate the DWARF sections appropriately.
   DwarfAbbrevSection = nullptr;   // SSUBTYP_DWABREV
   DwarfInfoSection = nullptr;     // SSUBTYP_DWINFO
   DwarfLineSection = nullptr;     // SSUBTYP_DWLINE

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

if (Subtarget.isAIXABI()) {
 
       // On AIX, direct function calls reference the symbol for the function's
       // entry point, which is named by prepending a "." before the function's
       // C-linkage name. A Qualname is returned here because an external
       // function entry point is a csect with XTY_ER property.
       const auto getExternalFunctionEntryPointSymbol = [&](StringRef SymName) {
         auto &Context = DAG.getMachineFunction().getMMI().getContext();
         MCSectionXCOFF *Sec = Context.getXCOFFSection(
-            (Twine(".") + Twine(SymName)).str(), XCOFF::XMC_PR, XCOFF::XTY_ER,
-            SectionKind::getMetadata());
+            (Twine(".") + Twine(SymName)).str(), SectionKind::getMetadata(),
+            XCOFF::CsectProperties(XCOFF::XMC_PR, XCOFF::XTY_ER));
         return Sec->getQualNameSymbol();
       };
 
       SymName = getExternalFunctionEntryPointSymbol(SymName)->getName().data();
     }
     return DAG.getTargetExternalSymbol(SymName, Callee.getValueType(),
                                        UsePlt ? PPCII::MO_PLT : 0);
   }