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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 9 inline comments as done.
DiggerLin added inline comments.


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


================
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;
----------------
hubert.reinterpretcast wrote:
> Suggestion: `CurrentAddressLocation`
changed as suggestion


================
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.
----------------
hubert.reinterpretcast wrote:
> I think the code can be made sufficiently self-explanatory that we don't need a comment here.
deleted the comment.


================
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.
----------------
hubert.reinterpretcast wrote:
> 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.
changed comment as suggestion


================
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;
----------------
hubert.reinterpretcast wrote:
> ```
> 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)
> ```
changed as suggestion.


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


================
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.
----------------
hubert.reinterpretcast wrote:
> 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.
changed as suggestion


================
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;
----------------
hubert.reinterpretcast wrote:
> 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.
changed , thanks


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-lcomm.ll:56
 ; SYMS-NEXT:   Symbol {
-; SYMS-NEXT:     Index: [[#Index:]]
-; SYMS-NEXT:     Name: a
+; SYMS:          Index: [[#Index:]]{{[[:space:]]*}}Name: a
 ; SYMS-NEXT:     Value (RelocatableAddress): 0x0
----------------
hubert.reinterpretcast wrote:
> It seems this file was changed accidentally by today's updates. The ` ` (space) character before the `*` is correct.
changed , thanks


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969