[PATCH] D95518: [Debug-Info][XCOFF] support dwarf for XCOFF for assembly output

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:276-278
+      // For csect section, it is storage mapping class.
+      // For debug section, it is section type flags.
+      unsigned Property;
----------------
The formulation of the class should fundamentally ensure that the csects and debug sections do not happen to share the same key. I don't think we should rely on the particular numeric values for that.


================
Comment at: llvm/include/llvm/MC/MCContext.h:280-281
 
-      XCOFFSectionKey(StringRef SectionName,
-                      XCOFF::StorageMappingClass MappingClass)
-          : SectionName(SectionName), MappingClass(MappingClass) {}
+      XCOFFSectionKey(StringRef SectionName, unsigned Property)
+          : SectionName(SectionName), Property(Property) {}
 
----------------
Add a new constructor instead of making the existing one less specific.


================
Comment at: llvm/include/llvm/MC/MCContext.h:284-285
       bool operator<(const XCOFFSectionKey &Other) const {
-        return std::tie(SectionName, MappingClass) <
-               std::tie(Other.SectionName, Other.MappingClass);
+        return std::tie(SectionName, Property) <
+               std::tie(Other.SectionName, Other.Property);
       }
----------------
Suggested corresponding update.


================
Comment at: llvm/include/llvm/MC/MCContext.h:586-590
     getXCOFFSection(StringRef Section, SectionKind K,
                     Optional<XCOFF::CsectProperties> CsectProp = None,
                     bool MultiSymbolsAllowed = false,
-                    const char *BeginSymName = nullptr);
+                    const char *BeginSymName = nullptr,
+                    unsigned SecFlags = 0);
----------------
Use `Optional` with a suitable enumeration type.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:38
   StringRef SymbolTableName;
+  unsigned SecFlags;
   bool MultiSymbolsAllowed;
----------------
Same comment re: using `Optional`.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:44-48
+                 unsigned SecFlags, MCSymbol *Begin, StringRef SymbolTableName,
                  bool MultiSymbolsAllowed)
       : MCSection(SV_XCOFF, Name, K, Begin),
         CsectProp(XCOFF::CsectProperties(SMC, ST)), QualName(QualName),
+        SymbolTableName(SymbolTableName), SecFlags(SecFlags),
----------------
This is the constructor used for csects.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:62-66
   MCSectionXCOFF(StringRef Name, SectionKind K, MCSymbolXCOFF *QualName,
-                 MCSymbol *Begin, StringRef SymbolTableName,
+                 unsigned SecFlags, MCSymbol *Begin, StringRef SymbolTableName,
                  bool MultiSymbolsAllowed)
       : MCSection(SV_XCOFF, Name, K, Begin), QualName(QualName),
+        SymbolTableName(SymbolTableName), SecFlags(SecFlags),
----------------
Suggested corresponding change.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:108
   bool isCsect() const { return CsectProp.hasValue(); }
+  unsigned getSecFlags() const { return SecFlags; }
 };
----------------
Suggested corresponding change.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1435-1436
   FileNo = FileNoOrErr.get();
-  if (NumFiles == Table.getMCDwarfFiles().size())
+  // For a new file, print a .file directive if the target supports it, otherwise
+  // return early.
+  if (NumFiles == Table.getMCDwarfFiles().size() ||
----------------
The comment only covers half the condition? That makes reading this confusing.


================
Comment at: llvm/lib/MC/MCContext.cpp:682-684
+      XCOFFSectionKey{Section.str(), IsDebugSec
+                                         ? SecFlags
+                                         : (unsigned)CsectProp->MappingClass},
----------------
Don't use the `{}` initialization for `XCOFFSectionKey` now that its constructors are more complicated.


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:16-18
-  assert(!getName().equals(getUnqualifiedName()) &&
-         "Symbol does not represent a csect; MCSectionXCOFF that represents "
-         "the symbol should not be (but is) set.");
----------------
This function (as named) is for csects. The assert should stay or some more extensive changes are needed.


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:30-32
-  assert(!getName().equals(getUnqualifiedName()) &&
-         "Symbol does not represent a csect; can only set a MCSectionXCOFF "
-         "representation for a csect.");
----------------
This function (as named) is for csects. The assert should stay or some more extensive changes are needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518