[PATCH] D114737: [lld][Macho] Include dead-stripped symbols in mapfile

[Roger Kim via Phabricator via llvm-commits]

Roger marked 2 inline comments as done.
Roger added inline comments.


================
Comment at: lld/MachO/MapFile.cpp:140-152
+    if (live) {
+      for (Symbol *sym : syms) {
+        assert(sym->isLive());
+        os << format("0x%08llX\t[%3u] %s\n", sym->getVA(),
+                     readerToFileOrdinal[sym->getFile()], symStr[sym].c_str());
+      }
+    } else {
----------------
int3 wrote:
> with the `if` being hoisted out, I'm not sure this `writeSymbols` function is really factoring out a useful amount of code. How about inlining it?
Done.


================
Comment at: lld/MachO/MapFile.cpp:160
 
-  // TODO: when we implement -dead_strip, we should dump dead stripped symbols
+  if (config->deadStrip) {
+    os << "# Dead Stripped Symbols:\n";
----------------
int3 wrote:
> I was wondering if this should be `!deadSymbols.empty()`, but it seems like `config->deadStrip` is the right one as ld64 will emit the "Dead Stripped Symbols" line as long as `-dead_strip` is passed. Can we have a test that checks for the case where no symbols get stripped?
I was also thinking about that. I figured it would be better to print the "Dead Stripped Symbols:" header even if there are no dead stripped symbols to explicitly show that there are none. Basing it off the configuration felt like it would make the code would behave with fewest  surprises.


================
Comment at: lld/MachO/MapFile.cpp:57
+  parallelSort(v.begin(), v.end(), [Live](Defined *a, Defined *b) {
+    return Live && a->getVA() != b->getVA() ? a->getVA() < b->getVA()
+                                            : a->getName() < b->getName();
----------------
Roger wrote:
> thevinster wrote:
> > Roger wrote:
> > > thevinster wrote:
> > > > Why do we need to propagate `Live` to the lambda? Does ld64 order dead stripped symbols by the name only?
> > > The ordering for live symbols is first by virtual address and then by name. As I understand, dead-stripped symbols don't have virtual addresses so we can just order by name.
> > Do we need to have this ordered? I guess you're trying to not fork the code so passing it into the lambda makes sense, but in general, we could skip the sort for dead stripped symbols. 
> I guess we don't need to have it ordered. It doesn't look like ld64 orders it. I can take out the ordering if that is better but I don't really know. I've refactored the code a bit but I don't know if that changes anything.
The code has changed enough where the original question no longer applies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114737