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

[int3]

Summary

... instead of mapping them to the intermediate object file.
This matches ld64.

[int3]

rm unused

[int3]

rm old comment

[Roger]

Looks good !

lld/MachO/MapFile.cpp

49

Could we make this name more specific? Like "SymbolEmittingFiles" or "EmittingFilese"?

54

can this function be made static?

lld/test/MachO/map-file.ll

6–7

For my benefit, could you explain more what this sentence means? Why would a "redundant object file" be and how would they be emitted?

12

As I understand, this flag makes the output into a dynamic library. Is this feature specific to dynamic libraries?

25–49

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?

[int3]

lld/MachO/MapFile.cpp

49

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

lld/test/MachO/map-file.ll

6–7

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'.

12

no, but I don't want to have to define main

25–49

added a comment above the definition of maybe_weak

lld/MachO/LTO.cpp

for (const lto::InputFile::Symbol &objSym : objSyms) {
 
     r.VisibleToRegularObj =
         sym->isUsedInRegularObj || (r.Prevailing && r.ExportDynamic);
 
     // Un-define the symbol so that we don't get duplicate symbol errors when we
     // load the ObjFile emitted by LTO compilation.
     if (r.Prevailing)
       replaceSymbol<Undefined>(sym, sym->getName(), sym->getFile(),
-                               RefState::Strong);
+                               RefState::Strong, /*wasBitcodeSymbol=*/true);
 
     // TODO: set the other resolution configs properly
   }
   checkError(ltoObj->add(std::move(f.obj), resols));
 }
 
 // Merge all the bitcode files we have seen, codegen the result
 // and return the resulting ObjectFile(s).

lld/MachO/MapFile.cpp

 #include "llvm/Support/Parallel.h"
 #include "llvm/Support/TimeProfiler.h"
 
 using namespace llvm;
 using namespace llvm::sys;
 using namespace lld;
 using namespace lld::macho;
 
-using Symbols = std::vector<Defined *>;
-// Returns a pair where the left element is a container of all live Symbols and
-// the right element is a container of all dead symbols.
-static std::pair<Symbols, Symbols> getSymbols() {
-  Symbols liveSymbols, deadSymbols;
+struct MapInfo {
+  SmallVector<InputFile *> files;
+  SmallVector<Defined *> liveSymbols;
+  SmallVector<Defined *> deadSymbols;
+};
+
+static MapInfo gatherMapInfo() {
+  MapInfo info;
   for (InputFile *file : inputFiles)
-    if (isa<ObjFile>(file))
-      for (Symbol *sym : file->symbols)
+    if (isa<ObjFile>(file) || isa<BitcodeFile>(file)) {
+      bool hasEmittedSymbol = false;
+      for (Symbol *sym : file->symbols) {
         if (auto *d = dyn_cast_or_null<Defined>(sym))
           if (d->isec && d->getFile() == file) {
             if (d->isLive()) {
               assert(!shouldOmitFromOutput(d->isec));
-              liveSymbols.push_back(d);
+              info.liveSymbols.push_back(d);
             } else {
-              deadSymbols.push_back(d);
-            }
-          }
-  parallelSort(liveSymbols.begin(), liveSymbols.end(),
+              info.deadSymbols.push_back(d);
+            }
+            hasEmittedSymbol = true;
+          }
+      if (hasEmittedSymbol)
+        info.files.push_back(file);
+  parallelSort(info.liveSymbols.begin(), info.liveSymbols.end(),
                [](Defined *a, Defined *b) {
                  return a->getVA() != b->getVA() ? a->getVA() < b->getVA()
                                                  : a->getName() < b->getName();
                });
   parallelSort(
-      deadSymbols.begin(), deadSymbols.end(),
+      info.deadSymbols.begin(), info.deadSymbols.end(),
       [](Defined *a, Defined *b) { return a->getName() < b->getName(); });
-  return {std::move(liveSymbols), std::move(deadSymbols)};
+  return info;
 }
 
 // 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<Symbol *, std::string>
 getSymbolStrings(ArrayRef<Defined *> syms) {
   std::vector<std::string> str(syms.size());
void macho::writeMapFile() {
 
   // Dump output path.
   os << format("# Path: %s\n", config->outputFile.str().c_str());
 
   // Dump output architecture.
   os << format("# Arch: %s\n",
                getArchitectureName(config->arch()).str().c_str());
 
+  MapInfo info = gatherMapInfo();
+
   // Dump table of object files.
   os << "# Object files:\n";
   os << format("[%3u] %s\n", 0, (const char *)"linker synthesized");
   uint32_t fileIndex = 1;
   DenseMap<lld::macho::InputFile *, uint32_t> readerToFileOrdinal;
-  for (InputFile *file : inputFiles) {
-    if (isa<ObjFile>(file)) {
-      os << format("[%3u] %s\n", fileIndex, file->getName().str().c_str());
-      readerToFileOrdinal[file] = fileIndex++;
-    }
-  }
+  for (InputFile *file : info.files) {
 
   // Dump table of sections
   os << "# Sections:\n";
   os << "# Address\tSize    \tSegment\tSection\n";
   for (OutputSegment *seg : outputSegments)
     for (OutputSection *osec : seg->getSections()) {
       if (osec->isHidden())
         continue;
 
       os << format("0x%08llX\t0x%08llX\t%s\t%s\n", osec->addr, osec->getSize(),
                    seg->name.str().c_str(), osec->name.str().c_str());
     }
 
   // Dump table of symbols
-  auto [liveSymbols, deadSymbols] = getSymbols();
-
   DenseMap<Symbol *, std::string> liveSymbolStrings =
-      getSymbolStrings(liveSymbols);
+      getSymbolStrings(info.liveSymbols);
   os << "# Symbols:\n";
   os << "# Address\tSize    \tFile  Name\n";
-  for (Defined *sym : liveSymbols) {
+  for (Defined *sym : info.liveSymbols) {
     assert(sym->isLive());
     os << format("0x%08llX\t0x%08llX\t[%3u] %s\n", sym->getVA(), sym->size,
                  readerToFileOrdinal[sym->getFile()],
                  liveSymbolStrings[sym].c_str());
   }
 
   if (config->deadStrip) {
     DenseMap<Symbol *, std::string> deadSymbolStrings =
-        getSymbolStrings(deadSymbols);
+        getSymbolStrings(info.deadSymbols);
     os << "# Dead Stripped Symbols:\n";
     os << "#        \tSize    \tFile  Name\n";
-    for (Defined *sym : deadSymbols) {
+    for (Defined *sym : info.deadSymbols) {
       assert(!sym->isLive());
       os << format("<<dead>>\t0x%08llX\t[%3u] %s\n", sym->size,
                    readerToFileOrdinal[sym->getFile()],
                    deadSymbolStrings[sym].c_str());
     }
   }
 }

lld/MachO/SymbolTable.cpp

if (auto *defined = dyn_cast<Defined>(s)) {
 
         dupSymDiags.push_back({make_pair(srcLoc1, srcFile1),
                                make_pair(srcLoc2, srcFile2), defined});
       }
 
     } else if (auto *dysym = dyn_cast<DylibSymbol>(s)) {
       overridesWeakDef = !isWeakDef && dysym->isWeakDef();
       dysym->unreference();
+    } else if (auto *undef = dyn_cast<Undefined>(s)) {
+      // Preserve the original bitcode file name (instead of using the object
+      // file name).
+      if (undef->wasBitcodeSymbol)
+        file = undef->getFile();
     }
     // Defined symbols take priority over other types of symbols, so in case
     // of a name conflict, we fall through to the replaceSymbol() call below.
   }
 
   // With -flat_namespace, all extern symbols in dylibs are interposable.
   // FIXME: Add support for `-interposable` (PR53680).
   bool interposable = config->namespaceKind == NamespaceKind::flat &&

 
 Symbol *SymbolTable::addUndefined(StringRef name, InputFile *file,
                                   bool isWeakRef) {
   auto [s, wasInserted] = insert(name, file);
 
   RefState refState = isWeakRef ? RefState::Weak : RefState::Strong;
 
   if (wasInserted)
-    replaceSymbol<Undefined>(s, name, file, refState);
+    replaceSymbol<Undefined>(s, name, file, refState,
+                             /*wasBitcodeSymbol=*/false);
   else if (auto *lazy = dyn_cast<LazyArchive>(s))
     lazy->fetchArchiveMember();
   else if (isa<LazyObject>(s))
     extract(*s->getFile(), s->getName());
   else if (auto *dynsym = dyn_cast<DylibSymbol>(s))
     dynsym->reference(refState);
   else if (auto *undefined = dyn_cast<Undefined>(s))
     undefined->refState = std::max(undefined->refState, refState);

lld/MachO/Symbols.h

public:
   }
   bool isTlv() const override;
 
   bool isExternal() const { return external; }
   bool isAbsolute() const { return isec == nullptr; }
 
   uint64_t getVA() const override;
 
+  // Returns the object file that this symbol was defined in. This value differs
+  // from `getFile()` if the symbol originated from a bitcode file.
+  ObjFile *getObjectFile() const;
+
   std::string getSourceLocation();
 
   // Ensure this symbol's pointers to InputSections point to their canonical
   // copies.
   void canonicalize();
 
   static bool classof(const Symbol *s) { return s->kind() == DefinedKind; }
 

 // a dylib symbol is referenced. As a DylibFile property, it indicates the kind
 // of referenced symbols contained within the file. If there are both weak
 // and strong references to the same file, we will count the file as
 // strongly-referenced.
 enum class RefState : uint8_t { Unreferenced = 0, Weak = 1, Strong = 2 };
 
 class Undefined : public Symbol {
 public:
-  Undefined(StringRefZ name, InputFile *file, RefState refState)
-      : Symbol(UndefinedKind, name, file), refState(refState) {
+  Undefined(StringRefZ name, InputFile *file, RefState refState,
+            bool wasBitcodeSymbol)
+      : Symbol(UndefinedKind, name, file), refState(refState),
+        wasBitcodeSymbol(wasBitcodeSymbol) {
     assert(refState != RefState::Unreferenced);
   }
 
   bool isWeakRef() const override { return refState == RefState::Weak; }
 
   static bool classof(const Symbol *s) { return s->kind() == UndefinedKind; }
 
   RefState refState : 2;
+  bool wasBitcodeSymbol;
 };
 
 // On Unix, it is traditionally allowed to write variable definitions without
 // initialization expressions (such as "int foo;") to header files. These are
 // called tentative definitions.
 //
 // Using tentative definitions is usually considered a bad practice; you should
 // write only declarations (such as "extern int foo;") to header files.

lld/MachO/Symbols.cpp

if (!isec->isFinal) {
     // function before its address is assigned. The thunking algorithm
     // knows that unfinalized functions will be out of range, so it is
     // expedient to return a contrived out-of-range address.
     return TargetInfo::outOfRangeVA;
   }
   return isec->getVA(value);
 }
 
+ObjFile *Defined::getObjectFile() const {
+  return isec ? dyn_cast_or_null<ObjFile>(isec->getFile()) : nullptr;
+}
+
 void Defined::canonicalize() {
   if (unwindEntry)
     unwindEntry = unwindEntry->canonical();
   if (isec)
     isec = isec->canonical();
 }
 
 std::string Defined::getSourceLocation() {

lld/MachO/SyntheticSections.cpp

if (auto *defined = dyn_cast<Defined>(sym)) {
       if (defined->isAbsolute())
         continue;
 
       // Constant-folded symbols go in the executable's symbol table, but don't
       // get a stabs entry.
       if (defined->wasIdenticalCodeFolded)
         continue;
 
-      InputSection *isec = defined->isec;
-      ObjFile *file = dyn_cast_or_null<ObjFile>(isec->getFile());
+      ObjFile *file = defined->getObjectFile();
       if (!file || !file->compileUnit)
         continue;
 
       symbolsNeedingStabs.emplace_back(defined, defined->isec->getFile()->id);
     }
   }
 
   llvm::stable_sort(symbolsNeedingStabs,

lld/test/MachO/map-file.ll

+; REQUIRES: x86
+; RUN: rm -rf %t; split-file %s %t
+
+;; Verify that we map symbols to their original bitcode input files, not the
+;; intermediate object files. Also verify that we don't emit these intermediate
+;; object files if no symbol needs to reference them. (Intermediate object files
+;; may still be referenced if they contain compiler-synthesized symbols, such
+;; as outlined functions. TODO: Test this case.)
+
+; RUN: opt -module-summary %t/foo.ll -o %t/foo.thinlto.o
+; RUN: opt -module-summary %t/bar.ll -o %t/bar.thinlto.o
+
+; RUN: %lld -dylib %t/foo.thinlto.o %t/bar.thinlto.o -o %t/foobar.thinlto -map \
+; RUN:   %t/foobar.thinlto.map
+; RUN: FileCheck %s --check-prefix=FOOBAR < %t/foobar.thinlto.map
+
+; RUN: %lld -dylib %t/bar.thinlto.o %t/foo.thinlto.o -o %t/barfoo.thinlto -map \
+; RUN:   %t/barfoo.thinlto.map
+; RUN: FileCheck %s --check-prefix=BARFOO < %t/barfoo.thinlto.map
+
+; RUN: llvm-as %t/foo.ll -o %t/foo.o
+; RUN: llvm-as %t/bar.ll -o %t/bar.o
+; RUN: %lld -dylib %t/foo.o %t/bar.o -o %t/foobar -map %t/foobar.map
+; RUN: FileCheck %s --check-prefix=LTO < %t/foobar.map
+
+; 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:
+; FOOBAR:      # Symbols:
+; FOOBAR-NEXT: # Address        Size             File  Name
+; FOOBAR-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  1] _foo
+; FOOBAR-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  2] _bar
+; FOOBAR-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  2] _maybe_weak
+
+; BARFOO:      # Path: {{.*}}{{/|\\}}map-file.ll.tmp/barfoo.thinlto
+; BARFOO-NEXT: # Arch: x86_64
+; BARFOO-NEXT: # Object files:
+; BARFOO-NEXT: [  0] linker synthesized
+; BARFOO-NEXT: [  1] {{.*}}{{/|\\}}map-file.ll.tmp/bar.thinlto.o
+; BARFOO-NEXT: [  2] {{.*}}{{/|\\}}map-file.ll.tmp/foo.thinlto.o
+; BARFOO-NEXT: # Sections:
+; BARFOO:      # Symbols:
+; BARFOO-NEXT: # Address        Size             File  Name
+; BARFOO-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  1] _bar
+; BARFOO-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  1] _maybe_weak
+; BARFOO-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  2] _foo
+
+; LTO:      # Path: {{.*}}{{/|\\}}map-file.ll.tmp/foobar
+; LTO-NEXT: # Arch: x86_64
+; LTO-NEXT: # Object files:
+; LTO-NEXT: [  0] linker synthesized
+; LTO-NEXT: [  1] {{.*}}{{/|\\}}map-file.ll.tmp/foo.o
+; LTO-NEXT: [  2] {{.*}}{{/|\\}}map-file.ll.tmp/bar.o
+; LTO-NEXT: # Sections:
+; LTO:      # Symbols:
+; LTO-NEXT: # Address        Size             File   Name
+; LTO-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  1]  _foo
+; LTO-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  2]  _bar
+; LTO-NEXT: 0x{{[0-9A-F]+}}  0x{{[0-9A-F]+}}  [  2]  _maybe_weak
+
+;--- foo.ll
+target triple = "x86_64-apple-darwin"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @foo() {
+  ret void
+}
+
+;; This is weak in foo.ll but non-weak in bar.ll, so the definition in bar.ll
+;; will always prevail. Check that the map file maps `maybe_weak` to bar.ll
+;; regardless of the object file input order.
+define weak_odr void @maybe_weak() {
+  ret void
+}
+
+;--- bar.ll
+target triple = "x86_64-apple-darwin"
+target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+define void @bar() {
+  ret void
+}
+
+define void @maybe_weak() {
+  ret void
+}