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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

hi @jasonliu , thanks a lot for your review.

> 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.

I think the layout of the object should not be done in `XCOFFObjectWriter`. In file `XCOFFObjectWriter`, we only call `executePostLayoutBinding` which theoretically should not do something to the layout?  Instead, if we have any specific requirement for the layout of the object, does it make more sense to implement it in another virtual function `finishLayout()` or in `MCAssembler::layout()`? This function is before the calling to `executePostLayoutBinding` in class `MCAssembler`.

> 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).

Hmm, I did a prototype to change the patch D97184 <https://reviews.llvm.org/D97184> to create a virtual function `writeSection()` in class `XCOFFObjectWriter` and override this function in class `DwarfSectionEntry` and `CsectSectionEntry`, so we can use a single interface to write sections for both control section and DWARF section. We can also do the same change for other interfaces like writing symbols. I gave this up to avoid to big changes and thought maybe it is better to do that in another patch.

> 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).

Yeah, this is also a good way if we are determined we don't need this refactoring.



================
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>>;
 
----------------
jasonliu wrote:
> If we are doing dynamic allocation for CsectGroup, why do we need to store pointers? 
If we use `CsectGroup` instead of a pointer, so we need to allocate a CsectGroup object in stack and then insert the `std::move`-d stack object to the deque? I am not sure the benefit of allocating the object in stack and in heap? Could you help to explain more? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99257