[PATCH] D66969: Output XCOFF object text section

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:343
+  writeSymbolName(SymbolRef.getName());
+  W.write<uint32_t>(SymbolOffset);
+  W.write<int16_t>(SectionRef.Index);
----------------
The symbol Address should be: CSectionRef.Address + SymbolOffset


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


================
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
----------------
DiggerLin wrote:
> 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 
It's not about efficiency. I think it's overkill to pass in a reference to SectionRef if all we need is 'Section Index'.


================
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
 
----------------
DiggerLin wrote:
> 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.
I prefer to have the patch reflect exactly what we need to change or test to achieve certain functionality. The extra bit here confuses people. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969