[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:31
+// important ones for us (right now) are:
+// .text --> contains program code.
+// .data --> contains intialized data, function descriptors and the TOC.
----------------
+ read-only data


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:32
+// .text --> contains program code.
+// .data --> contains intialized data, function descriptors and the TOC.
+// .bss  --> contains uninitialized data.
----------------
s/intialized/initialized/;
Add a comma before "and".


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:37
+// data, and acts as a container for symbols. A csect is mapped
+// into a section based on its storage-mapping class.
+//
----------------
There are `.bss` and `.data` section csects with `XMC_RW` storage mapping class.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:50
+
+// Packs the csects alignment and type into a byte.
+uint8_t getEncodedType(const MCSectionXCOFF *);
----------------
s/csects/csect's/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:55
+struct Symbol {
+  const MCSymbolXCOFF *MCSym;
+  uint32_t SymbolTableIndex;
----------------
Does this need to be modifiable? If not, please make this `const`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:71
+struct ControlSection {
+  const MCSectionXCOFF *MCCSect;
+  uint32_t SymbolTableIndex;
----------------
Does this need to be modifiable? If not, please make this `const`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:85
+// sections contain csects, and some sections contain csects which are better
+// stored seperatly eg: the .data section containing read-write, descriptor,
+// TOCBase and TOC-entry csects.
----------------
s/seperatly/separately/;
s/ eg:/, e.g.,/;




================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:285
 
-  // TODO FIXME Assign section numbers/finalize sections.
+  writeFileHeader();
+  writeSectionHeaderTable();
----------------
Is the plan to have 64-bit versions of the functions? If the plan is to use the same functions, then having 64-bit guards on all of them now might make sense.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:342
+
+  // BSS Section is unique in that the csects contain a single symbol, and the
+  // contained symbol does not need a label definition.
----------------
"The" before "BSS Section".

s/unique/special/;

External references behave similarly, and it is permitted to generate other csects "[containing] a single symbol" without a label definition by emitting the csect with a form of external linkage and using the linkage name of the symbol for the csect.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:343
+  // BSS Section is unique in that the csects contain a single symbol, and the
+  // contained symbol does not need a label definition.
+  for (auto &Sec : BSSCSects) {
----------------
Missing "the" before "contained symbol".
s/does not need/cannot be represented in the symbol table as/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:349
+
+    // Write the symbols name.
+    if (Sym.nameInStringTable()) {
----------------
s/symbols/symbol's/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:369
+
+    W.write<uint8_t>(Sym.getStorageClass());
+    // Always 1 aux entry for now.
----------------
Newline after this to clarify the binding of the comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:373
+
+    W.write<uint32_t>(Sec.Size);
+    // Parameter typecheck hash. Not supported.
----------------
Newline after this to clarify the binding of the comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:428
+
+    // Pad out Address to a 4 byte alignment. This is to match how the system
+    // assembler handles the .bss section. It's size is always a multiple of 4.
----------------
s/4 byte/4-byte/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:429
+    // Pad out Address to a 4 byte alignment. This is to match how the system
+    // assembler handles the .bss section. It's size is always a multiple of 4.
+    Address = alignTo(Address, SectionSizeMultiple);
----------------
s/It's/Its/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:453
+// Takes the log base 2 of the alignment and shifts the result into the 5 most
+// significant  bits of a byte, then or's in the csect type into the least
+// significant 3 bits.
----------------
Fix the consecutive spaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159