[XCOFF] [NFC] make StorageMappingClass/SymbolType member optional in MCSectionXCOFF class

[shchenz]

Summary

This is a follow-up of D95931. In that patch, we made the function getXCOFFSection has optional parameters for StorageMappingClass and SymbolType.

This patch makes
1: the private class members MappingClass and Type optional;
2: add an override version constructor function that has no MappingClass and Type properties.

Non-csect sections like debug sections have no such properties.

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCSectionXCOFF.h

35–36

I would suggest storing a XCOFF::CsectProperties member. That semantically reflects that these are either both present or both non-present (which is the case at the level of the object file encoding as well).

44–45

I suggest leaving the this constructor alone and adding an override the omits the csect properties.

88

It seems (from the code later in this patch) that the return type of this doesn't need to be Optional; instead, this function should assert that the current object here represents a csect.

91–95

Same comment about using an assert as above. Also, it would make sense to have an isCsect function.

llvm/lib/MC/XCOFFObjectWriter.cpp

608 ↗

(On Diff #323521)

The function is already named as being csect-specific. I suggest leaving this function signature alone and adding a new function interfaces for the specific non-csect cases.

894 ↗

(On Diff #323521)

I think the pointer-like interface is less mental overhead when reading the code.

[MaskRay]

llvm/lib/MC/MCSectionXCOFF.cpp

42

assert is not needed because getValue() would fail too

[hubert.reinterpretcast]

llvm/lib/MC/MCSectionXCOFF.cpp

42

I envision this patch ending up with some asserts on isCsect because custom asserts are friendlier when they trigger.

[shchenz]

1: make the get function as non Optional

[shchenz]

1: add isCsect member function

[shchenz]

Thanks for your review @hubert.reinterpretcast @MaskRay

Since now the get functions are not optional, so it should not impact the callers of them. But we need to revisit these functions when we add object output XCOFF DWARF support. I will leave the handlings in that follow up patch.

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCSectionXCOFF.h

35–36

If IsCsect is true exactly when CsectProp is not None, then I suggest just using CsectProp and having isCsect() return static_cast<bool>(CsectProp).

[shchenz]

1: eliminates the redundant IsCsect member

llvm/include/llvm/MC/MCSectionXCOFF.h

35–36

Thanks, updated.

[hubert.reinterpretcast]

LGTM with minor comments.

llvm/include/llvm/MC/MCSectionXCOFF.h

56

Minor nit: Might as well use the locally-available variable without the extra abstraction.

67

Please insert an empty line before comments like this to clearly indicate the associate of the comment with the following line.

69

Same comment re: empty line.

84

Minor nit: isCsect() has a friendlier name.

92

Same comment as above.

llvm/lib/MC/MCSectionXCOFF.cpp

79

Same comment as above.

[shchenz]

1: address @hubert.reinterpretcast comments

[shchenz]

Thanks for your review @hubert.reinterpretcast

llvm/include/llvm/MC/MCSectionXCOFF.h

 // 2) Uninitialized: The Type will be XTY_CM, it will contain a single symbol,
 //    and may not contain label definitions.
 // 3) An external reference providing a symbol table entry for a symbol
 //    contained in another XCOFF object file. External reference csects are not
 //    implemented yet.
 class MCSectionXCOFF final : public MCSection {
   friend class MCContext;
 
-  XCOFF::StorageMappingClass MappingClass;
-  XCOFF::SymbolType Type;
+  Optional<XCOFF::CsectProperties> CsectProp;
   MCSymbolXCOFF *const QualName;
   StringRef SymbolTableName;
   bool MultiSymbolsAllowed;
   static constexpr unsigned DefaultAlignVal = 4;
 
   MCSectionXCOFF(StringRef Name, XCOFF::StorageMappingClass SMC,
                  XCOFF::SymbolType ST, SectionKind K, MCSymbolXCOFF *QualName,
                  MCSymbol *Begin, StringRef SymbolTableName,
                  bool MultiSymbolsAllowed)
-      : MCSection(SV_XCOFF, Name, K, Begin), MappingClass(SMC), Type(ST),
-        QualName(QualName), SymbolTableName(SymbolTableName),
+      : MCSection(SV_XCOFF, Name, K, Begin),
+        CsectProp(XCOFF::CsectProperties(SMC, ST)), QualName(QualName),
+        SymbolTableName(SymbolTableName),
         MultiSymbolsAllowed(MultiSymbolsAllowed) {
-    assert((ST == XCOFF::XTY_SD || ST == XCOFF::XTY_CM || ST == XCOFF::XTY_ER) &&
-           "Invalid or unhandled type for csect.");
+    assert(
+        (ST == XCOFF::XTY_SD || ST == XCOFF::XTY_CM || ST == XCOFF::XTY_ER) &&
     assert(QualName != nullptr && "QualName is needed.");
     QualName->setRepresentedCsect(this);
     QualName->setStorageClass(XCOFF::C_HIDEXT);
     // A csect is 4 byte aligned by default, except for undefined symbol csects.
-    if (Type != XCOFF::XTY_ER)
-      setAlignment(Align(DefaultAlignVal));
+    if (CsectProp->Type != XCOFF::XTY_ER)
+      setAlignment(Align(DefaultAlignVal));
+  }
+
+  MCSectionXCOFF(StringRef Name, SectionKind K, MCSymbolXCOFF *QualName,
+                 MCSymbol *Begin, StringRef SymbolTableName,
+                 bool MultiSymbolsAllowed)
+      : MCSection(SV_XCOFF, Name, K, Begin), QualName(QualName),
+        SymbolTableName(SymbolTableName),
+        MultiSymbolsAllowed(MultiSymbolsAllowed) {
+    assert(QualName != nullptr && "QualName is needed.");
+    // FIXME: use a more meaningful name for non csect sections.
+    QualName->setRepresentedCsect(this);
+    // Set default alignment 4 for all non csect sections for now.
+    // FIXME: set different alignments according to section types.
   }
 
   void printCsectDirective(raw_ostream &OS) const;
 
 public:
   ~MCSectionXCOFF();
 
   static bool classof(const MCSection *S) {
     return S->getVariant() == SV_XCOFF;
   }
 
-  XCOFF::StorageMappingClass getMappingClass() const { return MappingClass; }
+  XCOFF::StorageMappingClass getMappingClass() const {
+    assert(CsectProp.hasValue() &&
+           "Only csect section has mapping class property!");
+    return CsectProp->MappingClass;
+  }
   XCOFF::StorageClass getStorageClass() const {
     return QualName->getStorageClass();
   }
-  XCOFF::SymbolType getCSectType() const { return Type; }
+  XCOFF::SymbolType getCSectType() const {
+    assert(CsectProp.hasValue() &&
+           "Only csect section has symbol type property!");
+    return CsectProp->Type;
+  }
   MCSymbolXCOFF *getQualNameSymbol() const { return QualName; }
 
   void PrintSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
                             raw_ostream &OS,
                             const MCExpr *Subsection) const override;
   bool UseCodeAlign() const override;
   bool isVirtualSection() const override;
   StringRef getSymbolTableName() const { return SymbolTableName; }
   bool isMultiSymbolsAllowed() const { return MultiSymbolsAllowed; }
+  bool isCsect() const { return CsectProp.hasValue(); }
 };
 
 } // end namespace llvm
 
 #endif

llvm/lib/MC/MCSectionXCOFF.cpp

void MCSectionXCOFF::PrintSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
   if (getKind().isReadOnly()) {
     if (getMappingClass() != XCOFF::XMC_RO)
       report_fatal_error("Unhandled storage-mapping class for .rodata csect.");
     printCsectDirective(OS);
     return;
   }
 
   if (getKind().isData()) {
     switch (getMappingClass()) {
     case XCOFF::XMC_RW:
     case XCOFF::XMC_DS:
       printCsectDirective(OS);
       break;
     case XCOFF::XMC_TC:
     case XCOFF::XMC_TE:
       break;
     case XCOFF::XMC_TC0:
if (getKind().isBSSLocal() || getKind().isCommon()) {
     return;
   }
 
   report_fatal_error("Printing for this SectionKind is unimplemented.");
 }
 
 bool MCSectionXCOFF::UseCodeAlign() const { return getKind().isText(); }
 
-bool MCSectionXCOFF::isVirtualSection() const { return XCOFF::XTY_CM == Type; }
+bool MCSectionXCOFF::isVirtualSection() const {
+  assert(CsectProp.hasValue() && "Only csect section can be virtual!");
+  return XCOFF::XTY_CM == CsectProp->Type;
+}