[PATCH] D67125: [PowerPC][AIX] Adds support for writing the data section in object files

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast requested changes to this revision.
hubert.reinterpretcast added a comment.
This revision now requires changes to proceed.

Generally, the problems copy/pasted from D66969 <https://reviews.llvm.org/D66969> will also need to be fixed in this patch.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:309
+  // Write the data csects one at a time.
+  for (const auto CSection : DataCsects)
+    Asm.writeSectionData(W.OS, CSection.MCCsect, Layout);
----------------
Please don't copy the `ControlSection`s.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:309
+  // Write the data csects one at a time.
+  for (const auto CSection : DataCsects)
+    Asm.writeSectionData(W.OS, CSection.MCCsect, Layout);
----------------
hubert.reinterpretcast wrote:
> Please don't copy the `ControlSection`s.
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.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:476
+  // Print out symbol table for the data section.
+  for (const auto CSection : DataCsects) {
+    // Write out the control section first and then each symbol in it.
----------------
Please don't copy the `ControlSection`s.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:538
+    Sections.push_back(&Data);
+    Data.Index = SectionIndex++;
+    Data.Address = SectionStartAddress;
----------------
In D66969, we try to check for overflow on the section index. We should do so here too to be consistent.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:542
+    for (auto &Csect : DataCsects) {
+      const MCSectionXCOFF *MCSec = Csect.MCCsect;
+      Address = alignTo(Address, MCSec->getAlignment());
----------------
This is the third copy of this code or thereabouts. At what point are we going to factor this code into a function?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:556
+    }
+    // Make sure the address of the next section aligned to DefaultSectionAlign.
+    Address = alignTo(Address, DefaultSectionAlign);
----------------
Suggestion: "Make sure that the address of the next section will be aligned to `DefaultSectionAlign`."


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:566
     BSS.Index = SectionIndex++;
-    uint32_t StartAddress = Address;
+    BSS.Address = SectionStartAddress;
+    uint32_t Address = SectionStartAddress;
----------------
This belongs in D66969 and not here. @DiggerLin, please fix.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67125