[mac/lld] Include archive name in diagnostics

[thakis]

Summary

Also, for .o files, include full path as given on link command line.

Before:

lld: error: undefined symbol [...], referenced from sandbox_logging.o

After:

lld: error: undefined symbol [...], referenced from libseatbelt.a(sandbox_logging.o)

Move archiveName up to InputFile so we can consistently use toString()
to print InputFiles in diags, and pass it to the ObjFile ctor. This
matches the ELF and COFF ports.

[thakis]

Lots of merge conflicts... :)

lld/MachO/InputFiles.cpp

419

Since the arg here is called err, was this supposed to call error() instead of warn()?

[MaskRay]

Reasonable

lld/MachO/InputFiles.h

72

Suggest: "If not empty, this stores the name of the archive containing this file. ..."

[thakis]

tweak comment

[thakis]

Thanks!

lld/MachO/InputFiles.cpp

419

Asked this on D89257 like I should have in the first place.

[MaskRay]

Thanks! This is what lld/ELF does...

[int3]

Thanks! I was actually thinking about this missing feature while implementing D91318: [lld-macho] Add archive name and file modtime to STABS output...

lld/MachO/Driver.cpp

282–289

nit: If we're going to add braces, I'd prefer we add them around all 3 levels of nesting

lld/MachO/InputFiles.h

74

why std::string over StringRef?

[thakis]

more braces

[thakis]

lld/MachO/InputFiles.h

74

Because it's a potential foot gun: addFile() sets this to path, which is a StringRef param and there's no guarantee that that stays alive. In fact, OPT_l calls findLibrary() which seems to return a temp string and pass that in. Since there are relatively few InputFiles, might as well not have a footgun here.

[thakis]

fix rebase weirdness

[int3]

sgtm

lld/MachO/InputFiles.h

74

bang bang yeah good point

[thakis]

Thanks!

lld/ELF/InputFiles.h

MutableArrayRef<Symbol *> getMutableSymbols() {
     assert(fileKind == BinaryKind || fileKind == ObjKind ||
            fileKind == BitcodeKind);
     return symbols;
   }
 
   // Get filename to use for linker script processing.
   StringRef getNameForScript() const;
 
-  // Filename of .a which contained this file. If this file was
-  // not in an archive file, it is the empty string. We use this
-  // string for creating error messages.
+  // If not empty, this stores the name of the archive containing this file.
+  // We use this string for creating error messages.
   std::string archiveName;
 
   // If this is an architecture-specific file, the following members
   // have ELF type (i.e. ELF{32,64}{LE,BE}) and target machine type.
   ELFKind ekind = ELFNoneKind;
   uint16_t emachine = llvm::ELF::EM_NONE;
   uint8_t osabi = 0;
   uint8_t abiVersion = 0;

lld/MachO/Driver.cpp

std::unique_ptr<object::Archive> file = CHECK(
         object::Archive::create(mbref), path + ": failed to parse archive");
 
     if (!file->isEmpty() && !file->hasSymbolTable())
       error(path + ": archive has no index; run ranlib to add one");
 
     if (config->allLoad || forceLoadArchive) {
       if (Optional<MemoryBufferRef> buffer = readFile(path)) {
         for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
-          auto file = make<ObjFile>(member.mbref, member.modTime);
-          file->archiveName = buffer->getBufferIdentifier();
-          inputFiles.push_back(file);
+          inputFiles.push_back(
+              make<ObjFile>(member.mbref, member.modTime, path));
         }
       }
     } else if (config->forceLoadObjC) {
       for (const object::Archive::Symbol &sym : file->symbols())
         if (sym.getName().startswith(objc::klass))
           symtab->addUndefined(sym.getName());
 
       // TODO: no need to look for ObjC sections for a given archive member if
       // we already found that it contains an ObjC symbol. We should also
       // consider creating a LazyObjFile class in order to avoid double-loading
       // these files here and below (as part of the ArchiveFile).
-      if (Optional<MemoryBufferRef> buffer = readFile(path))
-        for (const ArchiveMember &member : getArchiveMembers(*buffer))
-          if (hasObjCSection(member.mbref))
-            inputFiles.push_back(make<ObjFile>(member.mbref, member.modTime));
+      if (Optional<MemoryBufferRef> buffer = readFile(path)) {
+        for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
+          if (hasObjCSection(member.mbref)) {
+            inputFiles.push_back(
+                make<ObjFile>(member.mbref, member.modTime, path));
+          }
+        }
+      }
     }
 
     newFile = make<ArchiveFile>(std::move(file));
     break;
   }
   case file_magic::macho_object:
-    newFile = make<ObjFile>(mbref, getModTime(path));
+    newFile = make<ObjFile>(mbref, getModTime(path), "");
     break;
   case file_magic::macho_dynamically_linked_shared_lib:
   case file_magic::macho_dynamically_linked_shared_lib_stub:
     newFile = make<DylibFile>(mbref);
     break;
   case file_magic::tapi_file: {
     if (Optional<DylibFile *> dylibFile = makeDylibFromTAPI(mbref))
       newFile = *dylibFile;
bool macho::link(llvm::ArrayRef<const char *> argsArr, bool canExitEarly,
   compileBitcodeFiles();
   replaceCommonSymbols();
 
   StringRef orderFile = args.getLastArgValue(OPT_order_file);
   if (!orderFile.empty())
     parseOrderFile(orderFile);
 
   if (config->outputType == MH_EXECUTE && isa<Undefined>(config->entry)) {
-    error("undefined symbol: " + config->entry->getName());
+    error("undefined symbol: " + toString(*config->entry));
     return false;
   }
 
   createSyntheticSections();
   symtab->addDSOHandle(in.header);
 
   for (opt::Arg *arg : args.filtered(OPT_sectcreate)) {
     StringRef segName = arg->getValue(0);

lld/MachO/InputFiles.h

enum Kind {
     BitcodeKind,
   };
 
   virtual ~InputFile() = default;
   Kind kind() const { return fileKind; }
   StringRef getName() const { return name; }
 
   MemoryBufferRef mb;
+
   std::vector<Symbol *> symbols;
   ArrayRef<llvm::MachO::section_64> sectionHeaders;
   std::vector<SubsectionMap> subsections;
   // Provides an easy way to sort InputFiles deterministically.
   const int id;
 
+  // If not empty, this stores the name of the archive containing this file.
+  // We use this string for creating error messages.
+  std::string archiveName;
+
 protected:
   InputFile(Kind kind, MemoryBufferRef mb)
       : mb(mb), id(idCount++), fileKind(kind), name(mb.getBufferIdentifier()) {}
 
   InputFile(Kind kind, const llvm::MachO::InterfaceFile &interface)
       : id(idCount++), fileKind(kind), name(saver.save(interface.getPath())) {}
 
   void parseSections(ArrayRef<llvm::MachO::section_64>);
private:
   const StringRef name;
 
   static int idCount;
 };
 
 // .o file
 class ObjFile : public InputFile {
 public:
-  explicit ObjFile(MemoryBufferRef mb, uint32_t modTime);
+  explicit ObjFile(MemoryBufferRef mb, uint32_t modTime, StringRef archiveName);
   static bool classof(const InputFile *f) { return f->kind() == ObjKind; }
 
   llvm::DWARFUnit *compileUnit = nullptr;
-  StringRef archiveName = "";
   const uint32_t modTime;
 
 private:
   void parseDebugInfo();
 };
 
 // command-line -sectcreate file
 class OpaqueFile : public InputFile {

lld/MachO/InputFiles.cpp

 
 using namespace llvm;
 using namespace llvm::MachO;
 using namespace llvm::support::endian;
 using namespace llvm::sys;
 using namespace lld;
 using namespace lld::macho;
 
+// Returns "<internal>", "foo.a(bar.o)", or "baz.o".
+std::string lld::toString(const InputFile *f) {
+  if (!f)
+    return "<internal>";
+  if (f->archiveName.empty())
+    return std::string(f->getName());
+  return (path::filename(f->archiveName) + "(" + path::filename(f->getName()) +
+          ")")
+      .str();
+}
+
 std::vector<InputFile *> macho::inputFiles;
 std::unique_ptr<TarWriter> macho::tar;
 int InputFile::idCount = 0;
 
 // Open a given file path and return it as a memory-mapped file.
 Optional<MemoryBufferRef> macho::readFile(StringRef path) {
   // Open a file.
   auto mbOrErr = MemoryBuffer::getFile(path);
OpaqueFile::OpaqueFile(MemoryBufferRef mb, StringRef segName,
   isec->file = this;
   isec->name = sectName.take_front(16);
   isec->segname = segName.take_front(16);
   const auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
   isec->data = {buf, mb.getBufferSize()};
   subsections.push_back({{0, isec}});
 }
 
-ObjFile::ObjFile(MemoryBufferRef mb, uint32_t modTime)
+ObjFile::ObjFile(MemoryBufferRef mb, uint32_t modTime, StringRef archiveName)
     : InputFile(ObjKind, mb), modTime(modTime) {
+  this->archiveName = std::string(archiveName);
+
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
   auto *hdr = reinterpret_cast<const mach_header_64 *>(mb.getBufferStart());
 
   if (const load_command *cmd = findCommand(hdr, LC_SEGMENT_64)) {
     auto *c = reinterpret_cast<const segment_command_64 *>(cmd);
     sectionHeaders = ArrayRef<section_64>{
         reinterpret_cast<const section_64 *>(c + 1), c->nsects};
     parseSections(sectionHeaders);

 
 void ObjFile::parseDebugInfo() {
   std::unique_ptr<DwarfObject> dObj = DwarfObject::create(this);
   if (!dObj)
     return;
 
   auto *ctx = make<DWARFContext>(
       std::move(dObj), "",
-      [&](Error err) { warn(getName() + ": " + toString(std::move(err))); },
+      [&](Error err) {
+        warn(toString(this) + ": " + toString(std::move(err)));
+      },
       [&](Error warning) {
-        warn(getName() + ": " + toString(std::move(warning)));
+        warn(toString(this) + ": " + toString(std::move(warning)));
       });
 
   // TODO: Since object files can contain a lot of DWARF info, we should verify
   // that we are parsing just the info we need
   const DWARFContext::compile_unit_range &units = ctx->compile_units();
   auto it = units.begin();
   compileUnit = it->get();
   assert(std::next(it) == units.end());
DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella)
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
   auto *hdr = reinterpret_cast<const mach_header_64 *>(mb.getBufferStart());
 
   // Initialize dylibName.
   if (const load_command *cmd = findCommand(hdr, LC_ID_DYLIB)) {
     auto *c = reinterpret_cast<const dylib_command *>(cmd);
     dylibName = reinterpret_cast<const char *>(cmd) + read32le(&c->dylib.name);
   } else {
-    error("dylib " + getName() + " missing LC_ID_DYLIB load command");
+    error("dylib " + toString(this) + " missing LC_ID_DYLIB load command");
     return;
   }
 
   // Initialize symbols.
   // TODO: if a re-exported dylib is public (lives in /usr/lib or
   // /System/Library/Frameworks), we should bind to its symbols directly
   // instead of the re-exporting umbrella library.
   if (const load_command *cmd = findCommand(hdr, LC_DYLD_INFO_ONLY)) {
     auto *c = reinterpret_cast<const dyld_info_command *>(cmd);
     parseTrie(buf + c->export_off, c->export_size,
               [&](const Twine &name, uint64_t flags) {
                 bool isWeakDef = flags & EXPORT_SYMBOL_FLAGS_WEAK_DEFINITION;
                 bool isTlv = flags & EXPORT_SYMBOL_FLAGS_KIND_THREAD_LOCAL;
                 symbols.push_back(symtab->addDylib(saver.save(name), umbrella,
                                                    isWeakDef, isTlv));
               });
   } else {
-    error("LC_DYLD_INFO_ONLY not found in " + getName());
+    error("LC_DYLD_INFO_ONLY not found in " + toString(this));
     return;
   }
 
   if (hdr->flags & MH_NO_REEXPORTED_DYLIBS)
     return;
 
   const uint8_t *p =
       reinterpret_cast<const uint8_t *>(hdr) + sizeof(mach_header_64);
MemoryBufferRef mb =
                 toMachOString(sym));
 
   uint32_t modTime = toTimeT(
       CHECK(c.getLastModified(), toString(this) +
                                      ": could not get the modification time "
                                      "for the member defining symbol " +
                                      toMachOString(sym)));
 
-  auto file = make<ObjFile>(mb, modTime);
-  file->archiveName = getName();
+  auto file = make<ObjFile>(mb, modTime, getName());
 
   symbols.insert(symbols.end(), file->symbols.begin(), file->symbols.end());
   subsections.insert(subsections.end(), file->subsections.begin(),
                      file->subsections.end());
 }
 
 BitcodeFile::BitcodeFile(MemoryBufferRef mbref)
     : InputFile(BitcodeKind, mbref) {
   obj = check(lto::InputFile::create(mbref));
 }
-
-// Returns "<internal>" or "baz.o".
-std::string lld::toString(const InputFile *file) {
-  return file ? std::string(file->getName()) : "<internal>";
-}

lld/MachO/LTO.cpp

for (unsigned i = 1; i != maxTasks; ++i)
       saveBuffer(buf[i], config->outputFile + Twine(i) + ".lto.o");
   }
 
   // TODO: set modTime properly
   std::vector<ObjFile *> ret;
   for (unsigned i = 0; i != maxTasks; ++i)
     if (!buf[i].empty())
       ret.push_back(
-          make<ObjFile>(MemoryBufferRef(buf[i], "lto.tmp"), /*modTime=*/0));
+          make<ObjFile>(MemoryBufferRef(buf[i], "lto.tmp"), /*modTime=*/0, ""));
 
   return ret;
 }

lld/MachO/Writer.cpp

for (InputSection *isec : inputSections) {
     // before Writer runs might be cleaner...
     if (isec->segname == segment_names::ld)
       continue;
 
     for (Reloc &r : isec->relocs) {
       if (auto *s = r.referent.dyn_cast<lld::macho::Symbol *>()) {
         if (isa<Undefined>(s))
           error("undefined symbol " + toString(*s) + ", referenced from " +
-                sys::path::filename(isec->file->getName()));
+                toString(isec->file));
         else
           target->prepareSymbolRelocation(s, isec, r);
       } else {
         assert(r.referent.is<InputSection *>());
         if (!r.pcrel)
           in.rebase->addEntry(isec, r.offset);
       }
     }
void Writer::createOutputSections() {
   }
 
   for (SyntheticSection *ssec : syntheticSections) {
     auto it = mergedOutputSections.find({ssec->segname, ssec->name});
     if (it == mergedOutputSections.end()) {
       if (ssec->isNeeded())
         getOrCreateOutputSegment(ssec->segname)->addOutputSection(ssec);
     } else {
-      error("section from " + it->second->firstSection()->file->getName() +
+      error("section from " + toString(it->second->firstSection()->file) +
             " conflicts with synthetic section " + ssec->segname + "," +
             ssec->name);
     }
   }
 }
 
 void Writer::assignAddresses(OutputSegment *seg) {
   addr = alignTo(addr, PageSize);

lld/test/MachO/invalid/undefined-symbol.s

 # REQUIRES: x86
-# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
-# RUN: not %lld -o %t %t.o 2>&1 | FileCheck %s -DBASENAME=%basename_t
-# CHECK: error: undefined symbol _foo, referenced from [[BASENAME]]
+# RUN: rm -rf %t
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/main.s -o %t/main.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %t/foo.s -o %t/foo.o
+# RUN: llvm-ar crs %t/foo.a %t/foo.o
+# RUN: not %lld -o /dev/null %t/main.o 2>&1 | \
+# RUN:     FileCheck %s -DSYM=_foo -DFILENAME=%t/main.o
+# RUN: not %lld -o /dev/null %t/main.o %t/foo.a 2>&1 | \
+# RUN:     FileCheck %s -DSYM=_bar -DFILENAME='foo.a(foo.o)'
+# RUN: not %lld -o /dev/null %t/main.o -force_load %t/foo.a 2>&1 | \
+# RUN:     FileCheck %s -DSYM=_bar -DFILENAME='foo.a(foo.o)'
+# CHECK: error: undefined symbol [[SYM]], referenced from [[FILENAME]]
 
+#--- foo.s
+.globl _foo
+.text
+_foo:
+  callq _bar
+  retq
+
+#--- main.s
 .globl _main
 .text
 _main:
   callq _foo
   movq $0, %rax
   retq