[PATCH] D98323: [lld-macho] implement options -map

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Yeah diffs should generally come with corresponding tests. This code is fairly self-contained, so there's little chance of rebase conflicts and no rush to land :)

You can run the existing tests under lld/macho via `llvm-lit -vva <filename>`. `-vva` will let you see what commands are being run to get a sense of how things work.



================
Comment at: lld/MachO/MapFile.cpp:9
+//
+// This file implements the -Map option. It shows lists in order and
+// hierarchically the outputFile, arch, input files, output sections and
----------------



================
Comment at: lld/MachO/MapFile.cpp:61-70
+static std::vector<Defined *> getSymbols() {
+  std::vector<Defined *> v;
+  for (InputFile *file : inputFiles)
+    if (isa<ObjFile>(file))
+      for (lld::macho::Symbol *sym : file->symbols)
+        if (auto *d = dyn_cast<Defined>(sym))
+          if (d->isec && d->getFile() == file)
----------------
this is fairly similar to `SymtabSection::finalizeContents`, with some minor differences. We should check if the logic can be factored out. I can help with that though :) could you leave a TODO for now?


================
Comment at: lld/MachO/MapFile.cpp:65
+    if (isa<ObjFile>(file))
+      for (lld::macho::Symbol *sym : file->symbols)
+        if (auto *d = dyn_cast<Defined>(sym))
----------------
we should check for `sym != nullptr` in this loop (we don't parse symbols in debug sections, so they will be null)

also JFYI, `lld::macho::` won't be necessary once D98149 lands


================
Comment at: lld/MachO/MapFile.cpp:73-74
+// Construct a map from symbols to their stringified representations.
+// Demangling symbols (which is what toString() does) is slow, so
+// we do that in batch using parallel-for.
+static DenseMap<macho::Symbol *, std::string>
----------------
this function isn't using parallel-for :) leave a TODO?


================
Comment at: lld/MachO/MapFile.cpp:84
+
+static llvm::StringRef getArchName(Architecture Arch) {
+  switch (Arch) {
----------------
llvm/TextAPI/MachO/Architecture.h defines `getArchitectureName()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98323