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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Sorry this took a while... got a bit sidetracked



================
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:
> Yes, we need to emit N_GSYM symbols for globals, and N_STSYM symbols for static variables. Those need to be linked in the debug info.
> 
> N_STSYM always has the address set to a valid address. The stab entry should have the "value" set to the address, and "sect" set to the right section index just like the N_FUN entry.
> 
> N_GSYM may or may not have the address filed in. If the address of the global can be calculated, the "value" should be set to the address, and "sect" set to the right section index just like the N_FUN entry. If the address can't be calculated, N_GSYM can be zero and we will need to find the global by matching up with the non STAB symbol whose name must match exactly (and that symbol will have "value" and "sect" set correctly).
> 
> 
Addressed in {D92366}. Though I'm not sure how to test the case where we have a defined global whose address can't be calculated. How does that situation arise?


================
Comment at: lld/MachO/SyntheticSections.cpp:653
+        ObjFile *file = dyn_cast<ObjFile>(isec->file);
+        assert(file);
+        if (!file->compileUnit)
----------------
int3 wrote:
> clayborg wrote:
> > Is this truly unreachable code? 
> The only file types are object files, bitcode files, archives, and dylibs. We extract ObjFiles from ArchiveFiles & create them from BitcodeFiles before processing their InputSections, so we shouldn't see either of those here. And we shouldn't be emitting debug info for functions that our output binary references from dylibs (I think... correct me if I'm wrong).
After thinking about it a bit more, we should really refactor things such that this cast isn't necessary. But it's a bit hairy, so not in this diff


================
Comment at: lld/MachO/SyntheticSections.cpp:657-659
+        if (lastFile == nullptr || lastFile != file) {
+          if (lastFile != nullptr)
+            emitEndSourceStab();
----------------
int3 wrote:
> clayborg wrote:
> > Is this list sorted by file? We don't want to be emitting multiple N_SO + N_OSO entries for the same source file. dsymutil will be a lot less efficient if we have a few entries for "/tmp/foo.cpp" and "/tmp/foo.o" and then a few function, then some other source + object file, and then "/tmp/foo.cpp" and "/tmp/foo.o" again.
> yeah sorting did occur to me but I thought I'd implement it later. It's probably quite straightforward though, so I can do it in this diff
Done in {D92366}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89257