[PATCH] D89257: [lld-macho] Emit STABS symbols for debugging, and drop debug sections

[Jez Ng via Phabricator via llvm-commits]

int3 marked 9 inline comments as done.
int3 added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:627
+    // lieu is only correct if .subsections_via_symbols is set.
+    stab.value = defined->isec->getSize();
+    stabs.emplace_back(std::move(stab));
----------------
clayborg wrote:
> This can't be wrong and must be correct. If it is wrong, dsymutil will link the wrong address ranges for each function and really bad DWARF will result.
I'd like to punt on this for now. The goal for now is not to get the STABS implementation to 100% correctness, but to create sort-of-working binaries for local testing. As mentioned in the commit message, I'm on the fence about the best way to store symbol size in LLD's current architecture, and I think it will be clearer once more of the linker has been implemented.


================
Comment at: lld/MachO/SyntheticSections.cpp:635
   for (Symbol *sym : symtab->getSymbols()) {
+    // TODO support other symbol types
     if (isa<Defined>(sym) || sym->isInGot() || sym->isInStubs()) {
----------------
clayborg wrote:
> This can be done by just declaring a global variable and not assigning it a value:
> ```
> $ cat main.c
> int global;
> 
> int main(int argc, const char **argv) {
>   return 0;
> }
> $ clang -c -g main.c
> $ clang -g main.o
> $ dsymutil -s a.out 
> ----------------------------------------------------------------------
> Symbol table for: 'a.out' (x86_64)
> ----------------------------------------------------------------------
> Index    n_strx   n_type             n_sect n_desc n_value
> ======== -------- ------------------ ------ ------ ----------------
> [     0] 00000035 64 (N_SO         ) 00     0000   0000000000000000 '/tmp/'
> [     1] 0000003b 64 (N_SO         ) 00     0000   0000000000000000 'main.c'
> [     2] 00000042 66 (N_OSO        ) 03     0001   000000005fc5ec13 '/private/tmp/main.o'
> [     3] 00000001 2e (N_BNSYM      ) 01     0000   0000000100003fa0
> [     4] 00000056 24 (N_FUN        ) 01     0000   0000000100003fa0 '_main'
> [     5] 00000001 24 (N_FUN        ) 00     0000   0000000000000016
> [     6] 00000001 4e (N_ENSYM      ) 01     0000   0000000000000016
> [     7] 0000005c 20 (N_GSYM       ) 00     0000   0000000000000000 '_global'
> [     8] 00000001 64 (N_SO         ) 01     0000   0000000000000000
> [     9] 00000002 0f (     SECT EXT) 01     0010   0000000100000000 '__mh_execute_header'
> [    10] 00000016 0f (     SECT EXT) 03     0000   0000000100004000 '_global'
> [    11] 0000001e 0f (     SECT EXT) 01     0000   0000000100003fa0 '_main'
> [    12] 00000024 01 (     UNDF EXT) 00     0100   0000000000000000 'dyld_stub_binder'
> ```
> Note how symbol 7 has no valid address even though the actual non debug symbol does symbol 10. If lld can always emit the valid address for the STAB entry, that is quite ok. Just noting that ld64 doesn't always know it for some reason (or what ever linker the clang driver is evoking these days...).
ah... yeah LLD is able to emit the address for BSS symbols. I've added a test for that particular case in the other diff


================
Comment at: lld/MachO/SyntheticSections.cpp:650
+      // XXX is it right to assume that all symbols in __text are function
+      // symbols?
+      if (isec->name == "__text") {
----------------
clayborg wrote:
> I think we really need to get this right. Any section can contain functions. Most of the time, functions are in __text, but not always.
addressed in {D92430}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89257