[PATCH] D66969: Output XCOFF object text section

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast requested changes to this revision.
hubert.reinterpretcast added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:53
 struct Symbol {
-  const MCSymbolXCOFF *const MCSym;
+  const MCSymbolXCOFF *MCSym;
   uint32_t SymbolTableIndex;
----------------
DiggerLin wrote:
> sfertile wrote:
> > What prompted the removal of the `const`?
> if define as const MCSymbolXCOFF *const MCSym; , it can not have a default assignment operator for the struct Symbol.  a default assignment operator will be need in the SmallVector<Symbol, 1> Syms;  it will have compile error .
Is this answer in relation to some specific use of `Syms` or its containing class?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:153
+  void writeSymbolTableEntryForSymbol(const Symbol &, const ControlSection &,
+                                      const int16_t, uint64_t);
+  void writeSymbolTableEntryForControlSection(const ControlSection &,
----------------
Stray top-level `const` on function parameter declaration on a function declaration.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:155
+  void writeSymbolTableEntryForControlSection(const ControlSection &,
+                                              const int16_t,
+                                              XCOFF::StorageClass);
----------------
Stray top-level `const` on function parameter declaration on a function declaration.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:293
+  // Write the program code control sections one at a time.
+  for (const auto CSection : ProgramCodeCsects)
+    Asm.writeSectionData(W.OS, CSection.MCCsect, Layout);
----------------
Please don't copy the `ControlSection`s.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:293
+  // Write the program code control sections one at a time.
+  for (const auto CSection : ProgramCodeCsects)
+    Asm.writeSectionData(W.OS, CSection.MCCsect, Layout);
----------------
hubert.reinterpretcast wrote:
> Please don't copy the `ControlSection`s.
We already use `Sec` and `Csect` for similar cases. I don't think we need to add `CSection` into the mix.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:298
+uint64_t XCOFFObjectWriter::writeObject(MCAssembler &Asm,
+                                        const llvm::MCAsmLayout &Layout) {
   // We always emit a timestamp of 0 for reproducibility, so ensure incremental
----------------
Is the `llvm::` qualification necessary?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:322
 
+bool XCOFFObjectWriter::putNameInStringTable(const StringRef &SymbolName) {
+  return SymbolName.size() >= XCOFF::NameSize;
----------------
`putNameInStringTable` sounds like an action. Maybe use `nameShouldBeInStringTable`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:323
+bool XCOFFObjectWriter::putNameInStringTable(const StringRef &SymbolName) {
+  return SymbolName.size() >= XCOFF::NameSize;
+}
----------------
Names of `NameSize` length are placed in the symbol table.

Please add appropriate testing. This is a regression that was not caught.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:326
+
+void XCOFFObjectWriter::writeSymbolName(const StringRef &SymbolName) {
+  if (putNameInStringTable(SymbolName)) {
----------------
This function is misnamed for what it does. It writes the symbol table entry name field, not the symbol name.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:448
 
-void XCOFFObjectWriter::writeSymbolTable() {
-  assert(ProgramCodeCsects.size() == 0 && ".text csects not handled yet.");
+void XCOFFObjectWriter::writeSymbolTable(const llvm::MCAsmLayout &Layout) {
+  // Print out symbol table for the program code.
----------------
Same question re: `llvm::` as before.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:450
+  // Print out symbol table for the program code.
+  for (const auto CSection : ProgramCodeCsects) {
+    // Write out the control section first and then each symbol in it.
----------------
Please don't copy the `ControlSection`s.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969