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

[Kellie Medlin via Phabricator via llvm-commits]

Ktwu marked 4 inline comments as done.
Ktwu added inline comments.


================
Comment at: lld/MachO/Writer.cpp:356
+
+void Writer::assignSymtabAddresses(uint64_t start) {
+  symtabSeg->symOff = start;
----------------
ruiu wrote:
> This function should be called `finalize` and should belong to LCSymtab class instead of Writer.  `nSyms` and `strSize` member variables as well as how to compute the size of symtab are internal details of the symtab section, and Write shouldn't take care of them.
> 
> In ELF, we call function in this order:
> 
> 1. For each output section, call finalize()
> 2. Call Writer::assignAddresses()
> 3. For each output section, call write()
> 
> (1) finalizes the contents of the section. Each section knows how to compute its size, and the size should be computed by finalize and shouldn't change afterwards.
Gotcha; I'll move the functionality to LCSymtab, change the function names, and make sure I don't use local buffers when writing out the contents since that's unnecessary.


================
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() {
----------------
smeenai wrote:
> 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.
Sure!


================
Comment at: lld/MachO/Writer.cpp:376
+void Writer::createSymtabContents() {
+  SmallVector<char, 128> stringTable;
+  raw_svector_ostream stringTableOs{stringTable};
----------------
smeenai wrote:
> 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.
Good point.


================
Comment at: lld/test/MachO/symtab.s:31
+# CHECK-NEXT:     Type: Section (0xE)
+# CHECK-NEXT:     Section: __data
+# CHECK-NEXT:     RefType:
----------------
smeenai wrote:
> How come you're not checking the section index for this one?
It differs from what the system ld emits for the section index -- this diff emits 0x2, ld emits 0x3

For lack of understanding for why that it at the moment I figured neglecting the value to check is better than checking against an anomaly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76742