[PATCH] D97184: [XCOFF] support DWARF for XCOFF for object output

[Jason Liu via Phabricator via llvm-commits]

jasonliu added a comment.

FYI, I decide to review this patch before D97049 <https://reviews.llvm.org/D97049> because I wouldn't know if the NFC make sense without the context of this patch.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:243
   // table.
-  std::array<SectionEntry *const, 3> Sections{{&Text, &Data, &BSS}};
+  std::vector<SectionEntry *> Sections;
 
----------------
Would SmallVector work here? I think SmallVector is generally more preferable then std::vector in llvm source.
Also, is it feasible to have pre-defined DWARF section entries as well? so that we don't need to worry about poping dwarf section when resetting. And it would be consistent with `CsectSectionEntry`s as well.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:333
+      delete Sec;
+      Sec = nullptr;
+    }
----------------
This reset for `Sections` is not clean. For any `DwarfSectionEntry`s, you only deleted what's in there. But your vector would still contain these new `nullptr`s.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:423-425
+      XCOFFSection *DwarfSec = new XCOFFSection(MCSec);
+      DwarfSectionEntry *SecEntry = new DwarfSectionEntry(
+          MCSec->getName(), MCSec->getDwarfSubtypeFlags().getValue(), DwarfSec);
----------------
Not a fan of raw `new`s, it's always hard to track the ownership and where the deletion happens. I would suggest to use unique_ptr or just don't use pointer at all if possible. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:581
   for (const auto *Section : Sections) {
-    assert(isa<CsectSectionEntry>(Section) && "Only csect entry is supported!");
-    auto *CsectEntry = dyn_cast<CsectSectionEntry>(Section);
-    // Nothing to write for this Section.
-    if (CsectEntry->Index == SectionEntry::UninitializedIndex ||
-        CsectEntry->IsVirtual)
-      continue;
-
-    // There could be a gap (without corresponding zero padding) between
-    // sections.
-    assert(CurrentAddressLocation <= CsectEntry->Address &&
-           "CurrentAddressLocation should be less than or equal to section "
-           "address.");
-
-    CurrentAddressLocation = CsectEntry->Address;
-
-    for (const auto *Group : CsectEntry->Groups) {
-      for (const auto &Csect : *Group) {
-        if (uint32_t PaddingSize = Csect.Address - CurrentAddressLocation)
-          W.OS.write_zeros(PaddingSize);
-        if (Csect.Size)
-          Asm.writeSectionData(W.OS, Csect.MCSec, Layout);
-        CurrentAddressLocation = Csect.Address + Csect.Size;
-      }
-    }
-
-    // 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 =
-            CsectEntry->Address + CsectEntry->Size - CurrentAddressLocation) {
-      W.OS.write_zeros(PaddingSize);
-      CurrentAddressLocation += PaddingSize;
-    }
+    if (auto *CsectEntry = dyn_cast<CsectSectionEntry>(Section))
+      writeSectionForControlSectionEntry(Asm, Layout, *CsectEntry,
----------------
Please address all the Lint comments. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:671
 
+void XCOFFObjectWriter::writeSymbolTableEntryForDwarfSection(
+    const XCOFFSection &DwarfSectionRef, int16_t SectionIndex) {
----------------
I think we should use llvm-readobj to actually read the symbol entry for testing here. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:774
+    } else {
+      assert(isa<CsectSectionEntry>(Sec) && "unsupported section type!");
+      W.write<uint32_t>(Sec->Address);
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1028
+      // We use address 0 for DWARF sections' Physical and Virtual Addresses.
+      // This address is used to tell where is the seciton in the final object.
+      // See writeSectionForDwarfSectionEntry().
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1030
+      // See writeSectionForDwarfSectionEntry().
+      DwarfEntry->Address = DwarfSect.Address =
+          alignTo(Address, MCSec->getAlignment());
----------------
It seems that it's possible for this address and the RawPointer to have mismatched address? As at here, we are aligning to `MCSec->getAlignment()`. But below when we get to dwarf sections, we would align the `RawPointer` to `DefaultSectionAlign`? 
I think this is a bit confusing at least.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1060
+    // Make sure RawPointer is aligned.
+    RawPointer = alignTo(RawPointer, DefaultSectionAlign);
     if (RawPointer > UINT32_MAX)
----------------
This is more of a question: 
I noticed that when doing csect calculations above, we would consider the padding as part of csect's size. But we do not consider the padding for dwarf sections to be part of the dwarf sections. Is it the behavior we derived from system assembler? Would it work if we just treat the paddings as part of dwarf sections?


================
Comment at: llvm/test/DebugInfo/XCOFF/empty.ll:8
+; RUN:   llvm-dwarfdump --all - | FileCheck %s --check-prefix=DWARF32
 
 source_filename = "1.c"
----------------
Could we also add a llvm-objdump test for this? 
I think that test is useful to show the relationship between the dwarf raw datas and the other csects, as well as their alignments. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97184