[PATCH] D66969: Output XCOFF object text section

[Digger via Phabricator via llvm-commits]

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


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


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:79
+  // TODO: Handle Fixups Later
+  if (Fixups.size() > 0)
+    report_fatal_error("Fixups (relocations) not implemented yet.");
----------------
jasonliu wrote:
> 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. 
OK, I delete the code 
  if (Fixups.size() > 0)
    report_fatal_error("Fixups (relocations) not implemented yet.");


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


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


================
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());
----------------
jasonliu wrote:
> See comment in writeSymbolName()
here is adding the symbol name into string table.


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


================
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)
----------------
sfertile wrote:
> `CSections` should either be `control sections` or `csects`.
changed as   suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:331
+void XCOFFObjectWriter::writeSymbolName(const StringRef &SymbolName) {
+  if (putNameInStringTable(SymbolName)) {
+    W.write<uint32_t>(0);
----------------
sfertile wrote:
> 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.
for the writeSymbolName here , it write the symbol name or symbol name offset (value )into the XCOFF symbol entry.   and I think Sean has give an  answer to jason's question.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:331
+void XCOFFObjectWriter::writeSymbolName(const StringRef &SymbolName) {
+  if (putNameInStringTable(SymbolName)) {
+    W.write<uint32_t>(0);
----------------
DiggerLin wrote:
> sfertile wrote:
> > 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.
> for the writeSymbolName here , it write the symbol name or symbol name offset (value )into the XCOFF symbol entry.   and I think Sean has give an  answer to jason's question.
yes, Strings.finalize() has been called in the  XCOFFObjectWriter::executePostLayoutBinding() . it is before the getOffset


================
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
----------------
jasonliu wrote:
> Do we need to pass in a reference to SectionRef if all we want is an 'Index' from it?
I think pass an SectionRef reference is same efficient as passing index 


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


================
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) {
----------------
sfertile wrote:
> Storage class should come from the csect.
OK,  changed


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


================
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
 
----------------
jasonliu wrote:
> Not sure how this change is related to this patch.
I think it is not matter here, If you strong want me to delete it , I can delete it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969