This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Adds support for writing the data section in object files
ClosedPublic

Authored by xingxue on Sep 3 2019, 12:56 PM.

Details

Summary

Adds support for generating the XCOFF data section in object files for global variables with initialization.

Diff Detail

Repository
rL LLVM

Event Timeline

xingxue created this revision.Sep 3 2019, 12:56 PM
jasonliu added inline comments.Sep 4 2019, 11:58 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
541

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.

sfertile added inline comments.Sep 4 2019, 6:31 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
308

minor nit: CSections --> control sections or csects.

478

Storage class should come from CSection.

DiggerLin added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
478

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.

480

ditto

539

there is assert in the https://reviews.llvm.org/D66969 after for (auto &Csect : ProgramCodeCsects) {
}
maybe we can remove this one.

541

The Address value will been changed in the if (!ProgramCodeCsects.empty()) { }
if there is Text Section.

xingxue updated this revision to Diff 219434.Sep 9 2019, 2:11 PM

Addressed comments:

  • use csects in comment.
  • use section storage class from CSection.
  • set the section start address as the address after the previous section.
xingxue marked 8 inline comments as done.Sep 9 2019, 2:14 PM
xingxue added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
480

Will follow up when changes in D66969 is updated.

541

Good catch, fixed.

xingxue updated this revision to Diff 219799.Sep 11 2019, 1:58 PM

Changed from Data to Data.Index.

hubert.reinterpretcast requested changes to this revision.Sep 12 2019, 5:22 PM

Generally, the problems copy/pasted from D66969 will also need to be fixed in this patch.

llvm/lib/MC/XCOFFObjectWriter.cpp
309

Please don't copy the ControlSections.

309

Please see comment on D66969:
We already use Sec and Csect for similar cases. I don't think we need to add CSection into the mix.

476

Please don't copy the ControlSections.

538

In D66969, we try to check for overflow on the section index. We should do so here too to be consistent.

542

This is the third copy of this code or thereabouts. At what point are we going to factor this code into a function?

556

Suggestion: "Make sure that the address of the next section will be aligned to DefaultSectionAlign."

566

This belongs in D66969 and not here. @DiggerLin, please fix.

This revision now requires changes to proceed.Sep 12 2019, 5:22 PM

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.

DiggerLin added inline comments.Sep 17 2019, 7:48 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
480

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
538

We stopped checking for overflow on the section index (at least for now). There is a limited number of sections that we will generate.

566

The review of D66969 ended up with the conclusion that adding SectionStartAddress does not help readability.

xingxue marked an inline comment as done.Sep 30 2019, 9:56 AM
xingxue added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
542

@jasonliu is preparing an NFC patch to common up this code.

DiggerLin added inline comments.Oct 1 2019, 10:49 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
309

changed , thanks

476

changed , thanks

556

changed

566

changed

566

changed , thanks

xingxue updated this revision to Diff 222776.Oct 2 2019, 2:26 AM
xingxue marked 3 inline comments as done.

Addressed comments:

  • Changed variable name CSection -> Csect;
  • Added the checking of section headers in the object file.
xingxue marked 12 inline comments as done.Oct 2 2019, 2:29 AM
xingxue updated this revision to Diff 226435.Oct 25 2019, 8:47 AM
xingxue set the repository for this revision to rL LLVM.

Revised based on changes in patch https://reviews.llvm.org/D69112.

daltenty added inline comments.Oct 28 2019, 8:10 AM
llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
1

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.

21

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?

67

Do we want to check this? We currently don't in the object writing case (and this overlaps with aix-xcoff-common.ll).

xingxue marked 3 inline comments as done.Oct 28 2019, 9:52 AM
xingxue added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
1

WIll do.

21

These are added to check if the addresses for .bss in the header are correct after the .data section.

67

Again, these are added to check if the addresses for .bss in the header are correct after the .data section.

daltenty requested changes to this revision.Oct 29 2019, 7:36 AM

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

This revision now requires changes to proceed.Oct 29 2019, 7:36 AM
sfertile added inline comments.Oct 29 2019, 7:53 AM
llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
18

Minor nit: No need to change the name of this global.

21

Lets add the other members from aix-xcoff-common.ll lit test, and perform the checking while dumping the object and then we can get rid of 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.

daltenty accepted this revision.Oct 29 2019, 8:35 AM

Other than the minor test cleanup proposed by @sfertile, LGTM

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.

This revision was not accepted when it landed; it landed in state Needs Revision.Oct 30 2019, 11:50 AM
This revision was automatically updated to reflect the committed changes.