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

[Greg Clayton via Phabricator via llvm-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So marking as needing changes because the size of the N_FUN entries must be correct. dsymutil creates an address map when remapping symbols with ranges and if the size of the N_FUN stabs entry is too large, it will cause major problems when linking the DWARF. We should also find a rock solid way to identify N_FUN entries if possible instead of relying on the section name being "__text".



================
Comment at: lld/MachO/SyntheticSections.cpp:610
+  stab.strx = stringTableSection.addString(saver.save(path.str()));
+  stab.desc = 1;
+  stabs.emplace_back(std::move(stab));
----------------
Sounds good!


================
Comment at: lld/MachO/SyntheticSections.cpp:617
+    StabsEntry stab(MachO::N_FUN);
+    stab.sect = 1;
+    stab.strx = stringTableSection.addString(defined->getName());
----------------
Sounds good.


================
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));
----------------
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.


================
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()) {
----------------
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...).


================
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") {
----------------
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.


================
Comment at: lld/MachO/SyntheticSections.cpp:659
+          if (lastFile != nullptr)
+            emitEndSourceStab();
+          lastFile = file;
----------------
nice


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89257