[PATCH] D66969: Output XCOFF object text section

[Digger via Phabricator via llvm-commits]

DiggerLin marked 7 inline comments as done.
DiggerLin added a comment.

In D66969#1671945 <https://reviews.llvm.org/D66969#1671945>, @hubert.reinterpretcast wrote:

> The lack of testing with regards to what gets written for the raw data of the `.text` XCOFF section ought to be addressed.


in the patch we cannot deal with the DS csect in function in void XCOFFObjectWriter::executePostLayoutBinding .  we can not generate a raw data of the '.text" XCOFF section.
there are upcoming patch which deal with the  DS csect , I think we will add the test in that patch @jasonliu.



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:338
+
+void XCOFFObjectWriter::writeSymbolTableEntryForSymbol(
+    const Symbol &SymbolRef, const ControlSection &CSectionRef,
----------------
hubert.reinterpretcast wrote:
> This function is misnamed. It neither handles all variations of the LLVM concept of "symbol" nor the same for the XCOFF concept of "symbol". Maybe use `writeSymbolTableEntryForCsectMemberLabel`.
changed as suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:325
 
+bool XCOFFObjectWriter::isNameInStringTable(const StringRef &SymbolName) {
+  return SymbolName.size() > XCOFF::NameSize;
----------------
hubert.reinterpretcast wrote:
> This really sounds like the name is already in the string table.
changed as suggestion 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:351
+  // table entries for a detailed description. Bits 0-3 is for symbol
+  // visibility. Bit 10 (0x0020) can be set if the symbol is a function in the
+  // old XCOFF32 interpretation. Otherwise bit 10 should be 0. Since we don't
----------------
hubert.reinterpretcast wrote:
> The old comment is fine here. Please switch back to the shorter version. This is a part of the documentation that requires careful reading in context (and is a bad idea to quote portions from).
changed as  suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:351
+  // table entries for a detailed description. Bits 0-3 is for symbol
+  // visibility. Bit 10 (0x0020) can be set if the symbol is a function in the
+  // old XCOFF32 interpretation. Otherwise bit 10 should be 0. Since we don't
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > The old comment is fine here. Please switch back to the shorter version. This is a part of the documentation that requires careful reading in context (and is a bad idea to quote portions from).
> changed as  suggestion
changed as suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:356
+  // TODO FIXME How to assert a symbol's visibilty is default?
+  // TODO We aren't supporting the old XCOFF32 interpretation, but rather
+  // setting the bit (which is optional in the new interpretation) when
----------------
hubert.reinterpretcast wrote:
> The second TODO should say:
> ```
> // TODO Set the function indicator (bit 10, 0x0020) for functions
> // when debugging is enabled.
> ```
changed as suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:391
+  // table entries for a detailed description. Bits 0-3 is for symbol
+  // visibility. Bit 10 (0x0020) can be set if the symbol is a function in the
+  // old XCOFF32 interpretation. Otherwise bit 10 should be 0. Since we don't
----------------
hubert.reinterpretcast wrote:
> Use the shorter version of the comment here too,
changed as suggestion


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969