[PATCH] D76742: [lld] Add basic symbol table output
Kellie Medlin via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 9 11:59:20 PDT 2020
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
More information about the llvm-commits
mailing list