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

Jason Liu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 19 14:05:53 PDT 2021


jasonliu added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:344
+
+  Sections.clear();
 
----------------
This technically don't work as well, because you would clear the predefined csect sections. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:243
   // table.
-  std::array<SectionEntry *const, 3> Sections{{&Text, &Data, &BSS}};
+  std::vector<SectionEntry *> Sections;
 
----------------
shchenz wrote:
> 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?
We choose to have current way of handling TEXT/DATA/BSS because we want to group certain csects together, and we don't want to use raw new/delete whenever possible. Therefore, we use the pre-defined sections.
For dwarf, do we need to group things together? If not, would it be better to treat dwarf sections as something special? For example, leave `Sections` as is. But create a new `SmallVector<DwarfSectionEntry> DwarfSects`. Notice that I'm not using pointers here, so no more raw new/delete as well. I don't see a huge value of putting csect sections and dwarf sections into the same Array, other than saving a few extra for loops. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:423-425
+      XCOFFSection *DwarfSec = new XCOFFSection(MCSec);
+      DwarfSectionEntry *SecEntry = new DwarfSectionEntry(
+          MCSec->getName(), MCSec->getDwarfSubtypeFlags().getValue(), DwarfSec);
----------------
shchenz wrote:
> 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?
Could we save `XCOFFSection` instead of `a pointer to XCOFFSection` inside of `DwarfSectionEntry`? That way, you don't need to new/delete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97184



More information about the llvm-commits mailing list