[PATCH] D66969: Output XCOFF object text section header and symbol entry for program code

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:518
+      Csect.Size = Layout.getSectionAddressSize(MCSec);
+      Address = Csect.Address +  Csect.Size;
+      Csect.SymbolTableIndex = SymbolTableIndex;
----------------
There's two spaces after the `+`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:296
+  // Write the program code control sections one at a time.
+  uint32_t PreCSectEndAddress = Text.Address;
+  uint32_t PaddingSize;
----------------
Suggestion: `CurrentAddressLocation`


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:299
+  for (const auto &Csect : ProgramCodeCsects) {
+    // PaddingSize = Virtual address of current CSect - Virtual end address of
+    // previous CSect.
----------------
I think the code can be made sufficiently self-explanatory that we don't need a comment here.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:302
+    PaddingSize = Csect.Address - PreCSectEndAddress;
+    if (PaddingSize)
+      W.OS.write_zeros(PaddingSize);
----------------
The above write to `PaddingSize` is only read by the `if` and the use inside the `if`.
```
if (uint32_t PaddingSize = Csect.Address - CurrentAddressLocation)
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:308
+
+  // Padding Size of Tail Section  = 
+  // Virtual end address of current Section - Virtual end address of last CSect.
----------------
Suggestion:
The size of the tail padding in a section is the end virtual address of the current section minus the the end virtual address of the last csect in that section.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:310
+  // Virtual end address of current Section - Virtual end address of last CSect.
+  if (ProgramCodeCsects.size()) {
+    PaddingSize = Text.Address + Text.Size - PreCSectEndAddress;
----------------
```
if (!ProgramCodeCsects.empty())
```
however, I suggest checking the section and not the group of csects (they aren't the same thing):
```
if (Text.Index != -1)
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:312
+    PaddingSize = Text.Address + Text.Size - PreCSectEndAddress;
+    if (PaddingSize)
+      W.OS.write_zeros(PaddingSize);
----------------
Same comment about the write to `PaddingSize`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:531
+
+    // First Csect of each section do not need padding zero. We need to
+    // adjust section virtual address to first Csect's address.
----------------
Use "csect" instead of "Csect" when using the term in an English context where the word would not be capitalized.

Suggestion:
The first csect of a section can be aligned by adjusting the virtual address of its containing section instead of writing zeroes into the object file.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:544
     BSS.Index = SectionIndex++;
-    assert(alignTo(Address, DefaultSectionAlign) == Address &&
-           "Improperly aligned address for section.");
-    uint32_t StartAddress = Address;
+    // We use alignment address of previous section as BSS start address.
+    BSS.Address = Address;
----------------
The difference in the calculation for the virtual address of the `.bss` section and that of the `.text` section might complicate efforts to common up the handling. Note that a change in how the virtual address of `.bss` is calculated is within the scope of this patch because it changes the value from being always zero.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969