[PATCH] D136380: [lld-macho] Map file should map symbols to their original bitcode file

[Jez Ng via Phabricator via llvm-commits]

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


================
Comment at: lld/MachO/MapFile.cpp:45
+struct MapInfo {
+  SmallVector<InputFile *> files;
+  SmallVector<Defined *> liveSymbols;
----------------
Roger wrote:
> Could we make this name more specific? Like "SymbolEmittingFiles" or "EmittingFilese"?
I don't really think that makes things clearer. This entire file contains logic around emitting the map file, and this struct itself is called `MapInfo`; adding 'emit' here is redundant. We're not writing "emitLiveSymbols" below after all. I think looking at the map file output (of which a sample is contained in the comment at the top of the file) should provide a strong hint as to why this vector exists


================
Comment at: lld/test/MachO/map-file.ll:5-6
+;; Verify that we map symbols to their original bitcode input files, not the
+;; intermediate object files. Also verify that we don't emit redundant object
+;; files if no symbol needs to reference them.
+
----------------
Roger wrote:
> For my benefit, could you explain more what this sentence means? Why would a "redundant object file" be and how would they be emitted?
The intermediate object files referenced in the previous sentence are usually unreferenced, since we are mapping the symbols to the original bitcode files instead. But they may be referenced if they contain a synthesized symbol such as an outlined function. (I'll add a TODO to test that case.)

Object files can also become unreferenced if all the symbols defined within are weak definitions that get overridden. But that's not being tested here.

I'll remove 'redundant' and just say 'intermediate object files'.


================
Comment at: lld/test/MachO/map-file.ll:11
+
+; RUN: %lld -dylib %t/foo.thinlto.o %t/bar.thinlto.o -o %t/foobar.thinlto -map \
+; RUN:   %t/foobar.thinlto.map
----------------
Roger wrote:
> As I understand, this flag makes the output into a dynamic library. Is this feature specific to dynamic libraries?
no, but I don't want to have to define `main`


================
Comment at: lld/test/MachO/map-file.ll:24-48
+; FOOBAR:      # Path: {{.*}}{{/|\\}}map-file.ll.tmp/foobar.thinlto
+; FOOBAR-NEXT: # Arch: x86_64
+; FOOBAR-NEXT: # Object files:
+; FOOBAR-NEXT: [  0] linker synthesized
+; FOOBAR-NEXT: [  1] {{.*}}{{/|\\}}map-file.ll.tmp/foo.thinlto.o
+; FOOBAR-NEXT: [  2] {{.*}}{{/|\\}}map-file.ll.tmp/bar.thinlto.o
+; FOOBAR-NEXT: # Sections:
----------------
Roger wrote:
> If I understand correctly, these two tests are there to show that the maybe_weak symbol gets attributed to the bar file regardless of the order of the input files (because maybe_weak symbol is not weak in the bar file). Could we write that down as a comment here?
added a comment above the definition of `maybe_weak`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136380