[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: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);
----------------
sfertile wrote:
> hubert.reinterpretcast wrote:
> > sfertile wrote:
> > > hubert.reinterpretcast wrote:
> > > > I believe it is more robust to use `Sec->getCSectType() <= 0x07u`.
> > > > 
> > > > Also:
> > > > s/Csect/CSect/;
> > > > s/3-bits/3 bits./;
> > > Updated. Most of the tools and documentation use either `CSECT` or `csect`.  In these error messages 'csect' is the first word so I am capitalizing it. Should we always stick to 'CSect', and why?
> > Why did we name the functions `CSect`? I don't see why we need different styles for how to capitalize "csect".
> I honestly don't know. I've seen all 3 version used recently (and pretty sure I've used all 3 myself). If there is an 'official' way to capitalize it then lets use that, other wise lets pick one and use it consistently through out llvm codebase. I don't care which format, as long as we are consistent.
Agreed on consistency. Unfortunately following the "most official" way does not help with consistency. The Assembler Language Reference capitalizes as CSECT; however, my understanding of the LLVM style is that it means we should use CSECT in comments/messages, and `Csect` in code. `as` does generate at least one message where the capitalization is "Csect". In any case, "CSect" seems to be the odd one out, which is unfortunate, because then we'd have to rename functions.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:321
+    W.write(NameRef);
+    // Write the Physical Address and Virtual Address. In an object file these
+    // are the same.
----------------
Newline before the comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:325
+    W.write<uint32_t>(Sec->Address);
+    W.write<uint32_t>(Sec->Size);
+    W.write<uint32_t>(Sec->FileOffsetToData);
----------------
Newline before this line.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:327
+    W.write<uint32_t>(Sec->FileOffsetToData);
+    // Relocation pointer and Lineno pointer. Not supported yet.
+    W.write<uint32_t>(0);
----------------
Newline before the comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:330
+    W.write<uint32_t>(0);
+    // Relocation and line-number counts. Not supported yet.
+    W.write<uint16_t>(0);
----------------
Newline before the comment.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:333
+    W.write<uint16_t>(0);
+    W.write<int32_t>(Sec->Flags);
+  }
----------------
Newline before this line.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:352
+    if (Sym.nameInStringTable()) {
+      W.write<uint32_t>(0);
+      W.write<uint32_t>(Strings.getOffset(Sym.getName()));
----------------
Not really consequential, but `llvm::object::XCOFFSymbolEntry::NameInStrTblType::Magic` is signed.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:367
+    // this is always zero.
+    // TODO FIXME How to assert a symbols visibilty is default?
+    W.write<uint16_t>(0);
----------------
s/visibilty/visibility/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:382
+    // Symbol type.
+    W.write<int8_t>(getEncodedType(Sec.MCCSect));
+    // Storage mapping class.
----------------
`uint8_t`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:384
+    // Storage mapping class.
+    W.write<int8_t>(Sec.MCCSect->getMappingClass());
+    // Reserved (x_stab).
----------------
`uint8_t`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:386
+    // Reserved (x_stab).
+    W.write<int32_t>(0);
+    // Reserved (x_snstab).
----------------
`uint32_t`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:388
+    // Reserved (x_snstab).
+    W.write<int16_t>(0);
+  }
----------------
`uint16_t`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:395
+  // The address corrresponds to the address of sections and symbols in the
+  // object file. Address 0 is immediatly after the file header and section
+  // header table.
----------------
The last sentence should be more explicitly worded as a statement of LLVM behaviour: We can place the shared (as opposed to thread-local) address 0 immediately after the section header table.

Note the spelling of "immediately", and the removal of mentioning the file header.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:400
+  uint16_t SectionIndex = 1;
+  // The first symbol table entry is for the file name. Which we are not
+  // emitting yet, so start at index 0.
----------------
s/Which we are not emitting yet/We are not emitting it yet/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:411
+    BSS.Index = SectionIndex++;
+    Address = alignTo(Address, DefaultSectionAlign);
+    uint32_t StartAddress = Address;
----------------
I think that this can be an assertion that the incoming `Address` is already aligned.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:428
+
+    SymbolTableEntryCount = SymbolTableIndex;
+
----------------
At this point, we should just assign to `SymbolTableEntryCount` once after assigning all of the symbol table indices (not just those of the `.bss` section). In other words, move this somewhere outside of the surrounding `if` statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159