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

[ChenZheng via Phabricator via llvm-commits]

shchenz planned changes to this revision.
shchenz added a comment.

thanks for your review @jasonliu 
I will post an NFC patch first to make csect be allocated dynamically.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:344
+
+  Sections.clear();
 
----------------
jasonliu wrote:
> This technically don't work as well, because you would clear the predefined csect sections. 
No, I think it will do the clean-up as expected. Here we should clear all sections including 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;
 
----------------
jasonliu wrote:
> jasonliu wrote:
> > 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. 
> The alternative is to change the handling of TEXT/DATA/BSS csect sections as you mentioned. But if we want to go that way, I strongly prefer to do it now instead of fixing it later. I don't think having pre-defined csects sections and pushing new dwarf sections into the same array would work well as it is. 
I prefer to use a single vector to contain DWARF sections and csect sections. We should leverage the abstraction of class `SectionEntry`. 

Will post a patch to refactor the predefined sections handling first to make them be dynamically allocated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97184