Adds support for generating the XCOFF data section in object files for global variables with initialization.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
598 | We assigned Index and Size to Data section. But we haven't assign Address to it. By default, the address would be 0. It's correct if you do not have a Text section. But once you have text section, the Data's address as 0 would wrong. |
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
532 | I have changed the interface of void writeSymbolTableEntryForSymbol(const Symbol &, const ControlSection &, const int16_t, uint64_t); void writeSymbolTableEntryForControlSection(const ControlSection &, const int16_t, XCOFF::StorageClass); in the https://reviews.llvm.org/D66969. I think here also need to modification based on the new API. | |
534 | ditto | |
596 | there is assert in the https://reviews.llvm.org/D66969 after for (auto &Csect : ProgramCodeCsects) { | |
598 | The Address value will been changed in the if (!ProgramCodeCsects.empty()) { } |
Addressed comments:
- use csects in comment.
- use section storage class from CSection.
- set the section start address as the address after the previous section.
Generally, the problems copy/pasted from D66969 will also need to be fixed in this patch.
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
336 | Please don't copy the ControlSections. | |
336 | Please see comment on D66969: | |
530 | Please don't copy the ControlSections. | |
607–608 | This belongs in D66969 and not here. @DiggerLin, please fix. | |
610 | In D66969, we try to check for overflow on the section index. We should do so here too to be consistent. | |
614 | This is the third copy of this code or thereabouts. At what point are we going to factor this code into a function? | |
628 | Suggestion: "Make sure that the address of the next section will be aligned to DefaultSectionAlign." |
The lack of testing with regards to what gets written for the raw data of the .data XCOFF section ought to be addressed. At the very least, some testing of the .data section header (especially the length) should be present. Proper testing should also verify what the base virtual address associated with the .bss section is.
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
534 | please do not use copy construct here. please change to for (const auto& Sym : CSection.Syms) { |
Just following up on my previous comments on this patch based on developments in D66969.
llvm/lib/MC/XCOFFObjectWriter.cpp | ||
---|---|---|
607–608 | The review of D66969 ended up with the conclusion that adding SectionStartAddress does not help readability. | |
610 | We stopped checking for overflow on the section index (at least for now). There is a limited number of sections that we will generate. |
Addressed comments:
- Changed variable name CSection -> Csect;
- Added the checking of section headers in the object file.
llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll | ||
---|---|---|
4 | nit: I think it's worth while to add a comment to the beginning of this test indicating what we are hoping to test (i.e. initialized data). We are starting to have a lot of overlapping test cases, and for simplicity it would be nice to state the intent up front. | |
25 | I'm a bit confused why we are adding this, we don't refer to them in the object writing case. If it's to make sure we don't start emitting uninitialized data into .text shouldn't we have put some check-not's in? | |
87 | Do we want to check this? We currently don't in the object writing case (and this overlaps with aix-xcoff-common.ll). |
I don't think that initialized global data should be coming out as label symbols (everything appears to be contained in a ".data" csect). As I understand it only static data should be glob'd into one big csect, as per the XCOFF spec "An initialized definition for a global data scalar or structure ... is contained in its own csect". So each of these globals should have their own csect, which is the behavior exhibited by XLC.
I've observed that GCC on AIX is currently exhibiting this initalized-globals-in-one-csect behavior, but as we might expect, it interferes with the linkers ability to garbage collect unneeded global data.
I believe this is also an issue in the assembly path in D66154
As per out of band discussion, we will revisit this behavior under an option flag in a future revision and adopt the GCC behavior for now.
Yep, I was expecting we would transition to the described behaviour once we implement support for --ffunction-sections and --fdata-sections.
minor nit: CSections --> control sections or csects.