[PATCH] D76742: [lld] Add basic symbol table output

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

@ruiu, does this look good to you now?



================
Comment at: lld/MachO/Writer.cpp:374
+// symbol table entry [i] corresponds to string table entry [i]
+// https://github.com/aidansteele/osx-abi-macho-file-format-reference#nlist_64
+void Writer::createSymtabContents() {
----------------
Idk if this is a useful reference to put in a comment, since it's a random person's GitHub copy of a document which may or may not stay alive. There's lots more official documentation of the Mach-O structures; in fact, LLVM has its own ones in http://llvm.org/doxygen/BinaryFormat_2MachO_8h_source.html#l00992. I'd say it's safe to omit this.


================
Comment at: lld/MachO/Writer.cpp:376
+void Writer::createSymtabContents() {
+  SmallVector<char, 128> stringTable;
+  raw_svector_ostream stringTableOs{stringTable};
----------------
This is called after `assignAddresses`, so we should know our exact string table size at this point, right? We could call `reserve` on it with that exact size.


================
Comment at: lld/MachO/Writer.cpp:391
+    // TODO support other symbol types
+    // TODO populate n_desc
+    if (auto defined = dyn_cast<Defined>(sym)) {
----------------
Change to nDesc


================
Comment at: lld/MachO/Writer.cpp:393
+    if (auto defined = dyn_cast<Defined>(sym)) {
+      nType = (N_EXT | N_SECT);
+      nSect = defined->isec->sectionIndex;
----------------
Nit: you don't need the parentheses.


================
Comment at: lld/test/MachO/symtab.s:25
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Value: 0x[[#MAIN_VALUE + 8]]
+# CHECK-NEXT:   }
----------------
Nice!


================
Comment at: lld/test/MachO/symtab.s:31
+# CHECK-NEXT:     Type: Section (0xE)
+# CHECK-NEXT:     Section: __data
+# CHECK-NEXT:     RefType:
----------------
How come you're not checking the section index for this one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76742