[PATCH] D66969: Output XCOFF object text section

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:79
+  // TODO: Handle Fixups Later
+  if (Fixups.size() > 0)
+    report_fatal_error("Fixups (relocations) not implemented yet.");
----------------
Not sure if this report_fatal_error (and the related test case) is really necessary. We could have some intermediate state that relocation handling is not ready yet, but we want the code to flow through and deal with relocation later on. 


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:83
+  MCDataFragment *DF;
+  DF = getOrCreateDataFragment(&STI);
+  DF->setHasInstructions(STI);
----------------
The declaration and assignment could be one line?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:235
+    // which sections don't need to be in the table and omit them.
+    if (putNameInStringTable(MCSec->getSectionName()))
+      Strings.add(MCSec->getSectionName());
----------------
See comment in writeSymbolName()


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:280
+    // entry add it to the string table.
+    if (putNameInStringTable(XSym->getName()))
+      Strings.add(XSym->getName());
----------------
See comment in writeSymbolName()


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:331
+void XCOFFObjectWriter::writeSymbolName(const StringRef &SymbolName) {
+  if (putNameInStringTable(SymbolName)) {
+    W.write<uint32_t>(0);
----------------
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)?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:348
+  W.write<uint32_t>(SymbolOffset);
+  W.write<int16_t>(SectionRef.Index);
+  // Basic/Derived type. See the description of the n_type field for symbol
----------------
Do we need to pass in a reference to SectionRef if all we want is an 'Index' from it?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:376
+void XCOFFObjectWriter::writeSymbolTableEntryForControlSection(
+    const ControlSection &CSectionRef, const Section &SectionRef,
+    XCOFF::StorageClass StorageClass) {
----------------
Same here for SectionRef.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-common.ll:18
 @f = common local_unnamed_addr global float 0.000000e+00, align 4
+ at comm = common local_unnamed_addr global double 0.000000e+00, align 8
 
----------------
Not sure how this change is related to this patch.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969