[PATCH] D66969: Output XCOFF object text section

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:41
+  default:
+    llvm_unreachable("Not implemented yet.");
+  }
----------------
This is likely better served as a `report_fatal_error`.


================
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).
----------------
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.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:296
+                                      const MCAsmLayout &Layout) {
+  // Write the program code CSections one at a time.
+  for (const auto CSection : ProgramCodeCsects)
----------------
`CSections` should either be `control sections` or `csects`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:331
+void XCOFFObjectWriter::writeSymbolName(const StringRef &SymbolName) {
+  if (putNameInStringTable(SymbolName)) {
+    W.write<uint32_t>(0);
----------------
jasonliu wrote:
> If what we are going to put into the String Table are only symbols from Symbol Table, does it make sense to add symbols into String Table at here only (and remove the ones in executePostLayoutBinding)?
I believe we can't call `getOffset` unitl the `StringTableBuilder` has been finalized.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:455
+    // Write out the control section first and then each symbol in it.
+    writeSymbolTableEntryForControlSection(CSection, Text, XCOFF::C_HIDEXT);
+    for (const auto Sym : CSection.Syms) {
----------------
Storage class should come from the csect.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:507
+    }
+    Text.Size = Address - StartAddress;
+  }
----------------
We should assert size is a multiple of DefaultSectionAlign.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969