[PATCH] D66969: Output XCOFF object text section

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:230
+    // Add the section name to the string table in case we need it.
+    // At this point we assume that the same rules apply to names for sections
+    // as would for names of symbols (ie >= XCOFF::NameSize).
----------------
DiggerLin wrote:
> sfertile wrote:
> > Why are we 'assuming'? The name length limitation is a property of the symbol table entries, so we know it must use the same rules.
> changed the comment.
I think the wording of the comment is a bit awkward. I'd be ok with no comment on this block (the code is kind-of self documenting), otherwise maybe the same comment to the one you added for symbols.Its worded pretty well:

```
  // If the name does not fit in the storage provided in the symbol table
  // entry, add it to the string table.
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:53
 struct Symbol {
-  const MCSymbolXCOFF *const MCSym;
+  const MCSymbolXCOFF *MCSym;
   uint32_t SymbolTableIndex;
----------------
What prompted the removal of the `const`?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:71
   SmallVector<Symbol, 1> Syms;
-
+  StringRef getSectionName() const { return MCCsect->getSectionName(); }
   ControlSection(const MCSectionXCOFF *MCSec)
----------------
minor nit: `getName()` instead to keep it consistent with the symbol wrapper class.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:341
+    const int16_t Index, uint64_t SymbolOffset) {
+  // n_name, n_zeros, n_offset
+  writeSymbolName(SymbolRef.getName());
----------------
The comment makes it seem like we are about to write three fields, n_name *and* n_zeros *and* n_offset.  We have a descriptive function name so I don't think we need a comment, but if you want one i think it needs to be clear we are writing either `name` or `zeros and string table offset`.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:344
+  W.write<uint32_t>(CSectionRef.Address + SymbolOffset);
+  W.write<int16_t>(Index);
+  // Basic/Derived type. See the description of the n_type field for symbol
----------------
jasonliu wrote:
> jasonliu wrote:
> > int16_t -> uint16_t?
> Sorry my bad. It should be int16_t.
I think there are negative section indexes for special symbols (-1 for absoulte symbols, -2 for some debugging symbols?). Unfortunately I forgot my logon to the AIX machine I have access too so I can't look up the correct system header to verify 100%.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:487
+    Text.Index = SectionIndex++;
+    uint32_t StartAddress = Address;
+    for (auto &Csect : ProgramCodeCsects) {
----------------
We should be assigning Text.Address even though that is expected to always be virtual/physical address 0 anyway.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969