[PATCH] D65159: [PowerPC][XCOFF] Adds support for writing the .bss section for 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.

I am still working through this version, but I think this might need another pass on an updated copy.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:44
 
+constexpr unsigned DefaultSectionAlign = 16;
+
----------------
My observations of the XLC and GCC behaviour on AIX does not support what this patch does with this value. Namely, it is observed that the physical/virtual address assigned to the `.bss` section is not necessarily 16-byte aligned.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:70
+// Wrapper for an MCSectionXCOFF.
+struct ControlSection {
+  const MCSectionXCOFF *MCCSect;
----------------
If we mean `ControlSection32`, then let us name it that. Otherwise, I suggest adding in the FIXMEs for 64-bit support now.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:317
+void XCOFFObjectWriter::writeSectionHeaderTable() {
+  for (const auto Sec : Sections) {
+    // Write Name.
----------------
I suspect `const auto *Sec` is intended. As it is, we get a const pointer to non-const.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:421
+      assert(Csect.Syms.size() == 1 &&
+             +"Csects in the BSS can only contain a single symbol.");
+      Csect.Syms[0].SymbolTableIndex = Csect.SymbolTableIndex;
----------------
  - The unary `+` looks to be stray.
  - s/Csects/CSects/;




================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:426
+    // Each of which has 2 symbol table entries.
+    SymbolTableEntryCount += BSSCSects.size() * 2;
+
----------------
Please indicate why `+=` and why the multiplication instead of using `SymbolTableIndex`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:435
+  // Calculate the RawPointer value for each section.
+  uint64_t RawPointer = sizeof(XCOFF::FileHeader32) +
+                        Sections.size() * sizeof(XCOFF::SectionHeader32);
----------------
I suggest adding in the auxiliary header size even if it is currently always zero.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:437
+                        Sections.size() * sizeof(XCOFF::SectionHeader32);
+  for (auto Sec : Sections) {
+    if (!Sec->IsVirtual) {
----------------
`auto *Sec` to be consistent (and I like to be explicit on expecting a pointer anyway).


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:445
+  // TODO Add in Relocation storage to the RawPointer Calculation.
+  // TODO What to align the SymbolTable too?
+  // TODO Error check that the number of symbol table entries fits in 32-bits
----------------
s/too/to/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:456
+uint8_t getEncodedType(const MCSectionXCOFF *Sec) {
+  unsigned Align = Sec->getAlignment();
+  assert(isPowerOf2_32(Align) && "Alignment must be a power of 2.");
----------------
I'm not sure where we should address this, but it seems both XLC and GCC generate at least 4-byte alignment by default.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:458
+  assert(isPowerOf2_32(Align) && "Alignment must be a power of 2.");
+  assert(((Sec->getCSectType() & 0xF8) == 0) && "Csect type exceeds 3-bits");
+  unsigned Log2Align = Log2_32(Align);
----------------
I believe it is more robust to use `Sec->getCSectType() <= 0x07u`.

Also:
s/Csect/CSect/;
s/3-bits/3 bits./;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:462
+  // significant bits. Shift this value into the 5 most significant bits, and
+  // or in the csect type.
+  uint8_t EncodedAlign = (Log2Align & 0xFF) << 3;
----------------
To improve clarity, use "OR" or "bitwise-or" to represent the operation.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:463
+  // or in the csect type.
+  uint8_t EncodedAlign = (Log2Align & 0xFF) << 3;
+  return EncodedAlign | Sec->getCSectType();
----------------
The `& 0xFF` does nothing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159