[PATCH] D65159: [PowerPC][XCOFF] Adds support for writing the .bss section for object files.

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:75
+  uint32_t SymbolTableIndex;
+  uint32_t Address;
+  uint32_t Size;
----------------
`uint64_t`, add FIXME, or rename.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:76
+  uint32_t Address;
+  uint32_t Size;
+
----------------
`uint64_t`, add FIXME, or rename.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:93
+  // these values are equivalent.
+  uint32_t Address;
+  uint32_t Size;
----------------
`uint64_t`, add FIXME, or rename.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:94
+  uint32_t Address;
+  uint32_t Size;
+  uint32_t FileOffsetToData;
----------------
Same comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:95
+  uint32_t Size;
+  uint32_t FileOffsetToData;
+  uint32_t FileOffsetToRelocations;
----------------
`int64_t`/`uint64_t`, add FIXME, or rename.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:96
+  uint32_t FileOffsetToData;
+  uint32_t FileOffsetToRelocations;
+  uint32_t RelocationCount;
----------------
Same comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:98
+  uint32_t RelocationCount;
+  uint32_t Flags;
+
----------------
We are coding to C++14, so because this may be converted to a `int32_t` (because that is the actual type according to the system headers), store this as an `int32_t`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:100
+
+  uint32_t Alignment;
+  uint16_t Index;
----------------
This `Alignment` is not a representation of an XCOFF field, so it should follow the convention for types representing alignment values in LLVM. See also D64790.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:103
+
+  // Virutal sections do not need storage allocated in the object file.
+  const bool IsVirtual;
----------------
s/Virutal/Virtual/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:125
 class XCOFFObjectWriter : public MCObjectWriter {
+  // Container for a set of csects with (approximately) the same storage mapping
+  // class. For example all the csects with a storage mapping class of `xmc_pc`
----------------
s/Container for/Type to be used for a container representing/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:126
+  // Container for a set of csects with (approximately) the same storage mapping
+  // class. For example all the csects with a storage mapping class of `xmc_pc`
+  // will get placed into the same container.
----------------
`XMC_PR`? There's no `XMC_PC` in the header I am looking at.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:132
   std::unique_ptr<MCXCOFFObjectTargetWriter> TargetObjectWriter;
+  StringTableBuilder Strings{StringTableBuilder::XCOFF};
+
----------------
https://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:167
+  // of the structures in the object file representation. Namely:
+  // *) Calculates physical/virtual addresses, raw_pointer offsets, and section
+  //    sizes.
----------------
s/raw_pointer/raw-pointer/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:170
+  // *) Assigns symbol table indices.
+  // *) Builds up the section header table, by adding any non-empty sections to
+  //    `Sections`.
----------------
Remove the comma.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:217
+    llvm::MCAssembler &Asm, const llvm::MCAsmLayout &Layout) {
+  // Maps the MC Section representation to its corrresponding ControlSection
+  // wrapper. Needed for finding the ControlSection to insert an MCSymbol into
----------------
s/corrresponding/corresponding/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:220
+  // from its containing MCSectionXCOFF.
+  DenseMap<MCSectionXCOFF const *, ControlSection *> WrapperMap;
+
----------------
The East `const` here is foreign in relation to the rest of this function.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:222
+
+  // Creates the csect wrapper in VecSec, adds to the map, and adjusts the
+  // alignment of the containing section accordingly.
----------------
`VecSec`? Do you mean `CSections`?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:240
+      assert(XCOFF::XTY_SD == MCSec->getCSectType() &&
+             "Only an initilized csect can contain program code.");
+      Add(&Text, ProgramCodeCSects, MCSec);
----------------
s/initilized/initialized/;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159