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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/InputSection.h:46
   uint32_t align = 1;
+  uint32_t n_sect = 0;
   uint32_t flags = 0;
----------------
Nit: we normally use lowerCamelCase instead of snake_case.


================
Comment at: lld/MachO/Writer.cpp:336
         addr += isec->getSize();
+        isec->n_sect = ++n_sect;
       }
----------------
Hmm. This is gonna be incorrect for multiple sections of the same name, which should get merged together. Section merging is currently broken, as you pointed out.

I think we need an OutputSection class, similar to OutputSection in LLD ELF, to handle section merging. We have OutputSegments, but the sections in a segment themselves need merging too. That's also a much better place for the `addr` (which is the address in the *output*, not the address from the input) and `n_sect` fields to go.


================
Comment at: lld/MachO/Writer.cpp:396
+  for (Symbol *sym : symtab->getSymbols()) {
+    uint8_t n_type = N_UNDF;
+    uint8_t n_sect = NO_SECT;
----------------
Nit: variable naming


================
Comment at: lld/MachO/Writer.cpp:400
+
+    if (auto defined = dyn_cast<Defined>(sym)) {
+      n_type = (N_EXT | N_SECT);
----------------
Do we need a TODO here for handling more types?


================
Comment at: lld/MachO/Writer.cpp:416
+    linkEditSeg->os << n_sect;                                // n_sect
+    endian::write<uint16_t>(os, 0, endianness::little);       // n_dest
+    endian::write<uint64_t>(os, n_value, endianness::little); // n_value
----------------
It's `n_desc`, and we need a TODO here for filling this out.


================
Comment at: lld/test/MachO/symtab_basic.s:15
+# CHECK-NEXT:     ]
+# CHECK-NEXT:     Value:
+# CHECK-NEXT:   }
----------------
I think we should check the values, to make sure they're written correctly. They should be consistent (they'll depend on the base address, but that should be fixed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76742