[PATCH] D99257: [XCOFF] dynamically allocating the section and csect group

[Jason Liu via Phabricator via llvm-commits]

jasonliu added a subscriber: DiggerLin.
jasonliu added a comment.

Thanks for working on this version of the patch. It's always good to have something concrete to talk about instead of imaging how it's going to look like when everything is in place.
I'm still not convinced that dynamically allocating csect groups is the way to go though. 
As you could see in the patch, the code are more complicated with dynamic allocation, and less efficient(doing more loopings to find the correct CsectGroup). 
Also, we now lose control of how the CsectGroup are going to layout inside of a section, and how each section is going to layout in the object file (It now comes in a first come first serve order I believe). 
This could bring some practical issues for us. For example, we may prefer to have layouts like: `text -> data -> bss -> tdata -> tbss -> dwarf raw datas`. But now, I think it's possible that we start having `text -> data -> tdata -> bss -> tbss -> dwarf raw datas`, which is not quite right when you have tdata cut in between of normal datas.
And I don't see a lot of benefit of having a single vector to contain DWARF sections and csect section. In D97184 <https://reviews.llvm.org/D97184>, we actually handles DWARF sections and csect sections quite differently (i.e. a large block of if/else statement for dwarf and csects).
I would still prefer only dynamically allocating DWARF raw datas, and have pre-defined csect sections. It's less of an issue to dynamically allocating DWARF raw datas because we do not really care the layout between different dwarf raw datas (as long as they are at the end of the object files).

@hubert.reinterpretcast @daltenty @DiggerLin What's your opinions on this? Should we have an offline discussion about the direction?



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:95
 using CsectGroup = std::deque<XCOFFSection>;
-using CsectGroups = std::deque<CsectGroup *>;
+using CsectGroups = std::deque<std::unique_ptr<CsectGroup>>;
 
----------------
If we are doing dynamic allocation for CsectGroup, why do we need to store pointers? 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:232
 
-  // CsectGroups. These store the csects which make up different parts of
-  // the sections. Should have one for each set of csects that get mapped into
-  // the same section and get handled in a 'similar' way.
-  CsectGroup UndefinedCsects;
-  CsectGroup ProgramCodeCsects;
-  CsectGroup ReadOnlyCsects;
-  CsectGroup DataCsects;
-  CsectGroup FuncDSCsects;
-  CsectGroup TOCCsects;
-  CsectGroup BSSCsects;
+  std::unique_ptr<CsectGroup> UndefinedCsects;
 
----------------
I don't think it's necessary to have a unique_ptr of UndefinedCsects as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99257