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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
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:
> 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.
changed , thanks


================
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.
----------------
hubert.reinterpretcast wrote:
> Please don't copy the `ControlSection`s.
changed , thanks


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


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:566
     BSS.Index = SectionIndex++;
-    uint32_t StartAddress = Address;
+    BSS.Address = SectionStartAddress;
+    uint32_t Address = SectionStartAddress;
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > This belongs in D66969 and not here. @DiggerLin, please fix.
> The review of D66969 ended up with the conclusion that adding `SectionStartAddress` does not help readability.
changed


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:566
     BSS.Index = SectionIndex++;
-    uint32_t StartAddress = Address;
+    BSS.Address = SectionStartAddress;
+    uint32_t Address = SectionStartAddress;
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > This belongs in D66969 and not here. @DiggerLin, please fix.
> > The review of D66969 ended up with the conclusion that adding `SectionStartAddress` does not help readability.
> changed
changed , thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67125