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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

@jasonliu Thanks for your comments. Please see my inline reply.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:243
   // table.
-  std::array<SectionEntry *const, 3> Sections{{&Text, &Data, &BSS}};
+  std::vector<SectionEntry *> Sections;
 
----------------
jasonliu wrote:
> 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.
Yeah, SmallVector should work.

For pre-defined DWARF sections, to be honest, I don't think it is a good idea. We should let the caller of the object writer tell the writer what sections we should write. In this way, we don't need to change the writer part when we add new DWARF/Csect sections later, for example when we add DWARF 5 sections. There are many of them, 20 without split sections and 30 with split sections. It would be not good to predefined all of them.

I think maybe the current DWARF section handling should make more sense? We may need to add a FIXME to the original TEXT/DATA/BSS Csect handling?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:333
+      delete Sec;
+      Sec = nullptr;
+    }
----------------
jasonliu wrote:
> 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.
Good catch.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:423-425
+      XCOFFSection *DwarfSec = new XCOFFSection(MCSec);
+      DwarfSectionEntry *SecEntry = new DwarfSectionEntry(
+          MCSec->getName(), MCSec->getDwarfSubtypeFlags().getValue(), DwarfSec);
----------------
jasonliu wrote:
> 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. 
Yeah, this is a side effect due to the inconsistent `DwarfSectionEntry` and `CsectSectionEntry` handling.
Because `CsectSectionEntry` are now all pre-defined as `XCOFFObjectWriter`'s class members, we can not use `unique_ptr` as element of `XCOFFObjectWriter::Sections`.
And because there are two child types of the Sections, so we must use the parent pointer as the element of `XCOFFObjectWriter::Sections`.

So that seems to leave us no other choice but to use raw `new`.
I think if we handle `DwarfSectionEntry` and `CsectSectionEntry` the same, predefining or dynamic allocating, we should get rid of this issue.

What do you think?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1030
+      // See writeSectionForDwarfSectionEntry().
+      DwarfEntry->Address = DwarfSect.Address =
+          alignTo(Address, MCSec->getAlignment());
----------------
jasonliu wrote:
> 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.
Yeah, agree this will cause some issue. I update the RawPointer calculation logic.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1060
+    // Make sure RawPointer is aligned.
+    RawPointer = alignTo(RawPointer, DefaultSectionAlign);
     if (RawPointer > UINT32_MAX)
----------------
jasonliu wrote:
> 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?
We can not treat the padding as part of the DWARF sections because in some DWARF sections, like .dwline/.dwinfo section, they will contain the section size info in the section content. I think we can not make the size in DWARF section and in section header table mismatch. (Please note there is 4-byte difference for 32-bit XCOFF due to the length field itself.)


================
Comment at: llvm/test/DebugInfo/XCOFF/empty.ll:8
+; RUN:   llvm-dwarfdump --all - | FileCheck %s --check-prefix=DWARF32
 
 source_filename = "1.c"
----------------
jasonliu wrote:
> 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. 
I add one cases containing two cases: one is for sections dump and one is for relocation table dump. I can not dump the symbol table as now all tools don't support that. Hope this is ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97184