[COFF] Store a pointer to the parent instead of just the parent name in InputFile

[zturner]

Summary

I'm trying to make LLD PDBs look more like Microsoft PDBs. While we can never be perfect, any small amount we can get closer saves a lot of time when debugging and adds confidence that our PDBs are "correct".

Currently I'm trying to create the list of modules in the same order that MSVC creates them. In order to do that, we need a little bit more info from an object file than just the name of its parent. So, rather than adding more and more fields to keep track of all this extra information, we can just store a pointer to the parent itself.

Currently there's no functional change here, we're just changing a StringRef to an ArchiveFile*. In subsequent patches I'll use this to sort the module list in the same order that link.exe does before writing the PDB.

[ruiu]

The problem of having an archive file pointer instead of an archive file name is that we don't (and don't have to) instantiate an archive file object for an archive given as an argument for /wholearchive. How do you deal with that?

[zturner]

In D49095#1156542, @ruiu wrote:

The problem of having an archive file pointer instead of an archive file name is that we don't (and don't have to) instantiate an archive file object for an archive given as an argument for /wholearchive. How do you deal with that?

It seems like I don't deal with that currently, however all the tests pass. So either that means when an argument is passed to /wholearchive, we never end up needing the archive name anyway, or it means that we're lacking test coverage somewhere. Do you know which it is? If it's the former, then I guess I don't need to do anything. If it's the latter, then is there any reason we *can't* instantiate an ArchiveFile object? The expensive thing is parsing the archive, which we have to do regardless.

[ruiu]

I think it is a lack of an appropriate test. I believe we construct an error message at runtime by concatenating archive file name with a object file name, so if we handle object files in an /wholearchive archive file as if they are outside of any archive, we'd generate a wrong error message.

There's no strong reason not to instantiate archive files for /wholearchive. It's just that it's currently a (small) waste of resource.

lld/COFF/Driver.h

 public:
   void link(llvm::ArrayRef<const char *> Args);
 
   // Used by the resolver to parse .drectve section contents.
   void parseDirectives(StringRef S);
 
   // Used by ArchiveFile to enqueue members.
   void enqueueArchiveMember(const Archive::Child &C, StringRef SymName,
-                            StringRef ParentName);
+                            ArchiveFile *Parent);
 
   MemoryBufferRef takeBuffer(std::unique_ptr<MemoryBuffer> MB);
 
 private:
   std::unique_ptr<llvm::TarWriter> Tar; // for /linkrepro
 
   // Opens a file. Path has to be resolved already.
   MemoryBufferRef openFile(StringRef Path);
private:
   // entry point name.
   StringRef findDefaultEntry();
   WindowsSubsystem inferSubsystem();
 
   void invokeMSVC(llvm::opt::InputArgList &Args);
 
   void addBuffer(std::unique_ptr<MemoryBuffer> MB, bool WholeArchive);
   void addArchiveBuffer(MemoryBufferRef MBRef, StringRef SymName,
-                        StringRef ParentName);
+                        ArchiveFile *Parent);
 
   void enqueuePath(StringRef Path, bool WholeArchive);
 
   void enqueueTask(std::function<void()> Task);
   bool run();
 
   std::list<std::function<void()>> TaskQueue;
   std::vector<StringRef> FilePaths;

lld/COFF/Driver.cpp

case file_magic::windows_resource:
     Resources.push_back(MBRef);
     break;
   case file_magic::archive:
     if (WholeArchive) {
       std::unique_ptr<Archive> File =
           CHECK(Archive::create(MBRef), Filename + ": failed to parse archive");
 
       for (MemoryBufferRef M : getArchiveMembers(File.get()))
-        addArchiveBuffer(M, "<whole-archive>", Filename);
+        addArchiveBuffer(M, "<whole-archive>", nullptr);
       return;
     }
     Symtab->addFile(make<ArchiveFile>(MBRef));
     break;
   case file_magic::bitcode:
     Symtab->addFile(make<BitcodeFile>(MBRef));
     break;
   case file_magic::coff_object:
enqueueTask([=]() {
     if (MBOrErr.second)
       error("could not open " + PathStr + ": " + MBOrErr.second.message());
     else
       Driver->addBuffer(std::move(MBOrErr.first), WholeArchive);
   });
 }
 
 void LinkerDriver::addArchiveBuffer(MemoryBufferRef MB, StringRef SymName,
-                                    StringRef ParentName) {
+                                    ArchiveFile *Parent) {
   file_magic Magic = identify_magic(MB.getBuffer());
   if (Magic == file_magic::coff_import_library) {
     Symtab->addFile(make<ImportFile>(MB));
     return;
   }
 
   InputFile *Obj;
   if (Magic == file_magic::coff_object) {
     Obj = make<ObjFile>(MB);
   } else if (Magic == file_magic::bitcode) {
     Obj = make<BitcodeFile>(MB);
   } else {
     error("unknown file type: " + MB.getBufferIdentifier());
     return;
   }
 
-  Obj->ParentName = ParentName;
+  Obj->Parent = Parent;
   Symtab->addFile(Obj);
   log("Loaded " + toString(Obj) + " for " + SymName);
 }
 
 void LinkerDriver::enqueueArchiveMember(const Archive::Child &C,
                                         StringRef SymName,
-                                        StringRef ParentName) {
+                                        ArchiveFile *Parent) {
   if (!C.getParent()->isThin()) {
     MemoryBufferRef MB = CHECK(
         C.getMemoryBufferRef(),
         "could not get the buffer for the member defining symbol " + SymName);
-    enqueueTask([=]() { Driver->addArchiveBuffer(MB, SymName, ParentName); });
+    enqueueTask([=]() { Driver->addArchiveBuffer(MB, SymName, Parent); });
     return;
   }
 
   auto Future = std::make_shared<std::future<MBErrPair>>(createFutureForFile(
       CHECK(C.getFullName(),
             "could not get the filename for the member defining symbol " +
                 SymName)));
   enqueueTask([=]() {
     auto MBOrErr = Future->get();
     if (MBOrErr.second)
       fatal("could not get the buffer for the member defining " + SymName +
             ": " + MBOrErr.second.message());
     Driver->addArchiveBuffer(takeBuffer(std::move(MBOrErr.first)), SymName,
-                             ParentName);
+                             Parent);
   });
 }
 
 static bool isDecorated(StringRef Sym) {
   return Sym.startswith("@") || Sym.contains("@@") || Sym.startswith("?") ||
          (!Config->MinGW && Sym.contains('@'));
 }
 

 void LinkerDriver::invokeMSVC(opt::InputArgList &Args) {
   std::string Rsp = "/nologo\n";
   std::vector<std::string> Temps;
 
   // Write out archive members that we used in symbol resolution and pass these
   // to MSVC before any archives, so that MSVC uses the same objects to satisfy
   // references.
   for (ObjFile *Obj : ObjFile::Instances) {
-    if (Obj->ParentName.empty())
+    if (!Obj->Parent)
       continue;
     SmallString<128> S;
     int Fd;
     if (auto EC = sys::fs::createTemporaryFile(
-            "lld-" + sys::path::filename(Obj->ParentName), ".obj", Fd, S))
+            "lld-" + sys::path::filename(Obj->Parent->getName()), ".obj", Fd, S))
       fatal("cannot create a temporary file: " + EC.message());
     raw_fd_ostream OS(Fd, /*shouldClose*/ true);
     OS << Obj->MB.getBuffer();
     Temps.push_back(S.str());
     Rsp += quote(S) + "\n";
   }
 
   for (auto *Arg : Args) {

lld/COFF/InputFiles.h

 using llvm::COFF::IMAGE_FILE_MACHINE_UNKNOWN;
 using llvm::COFF::MachineTypes;
 using llvm::object::Archive;
 using llvm::object::COFFObjectFile;
 using llvm::object::COFFSymbolRef;
 using llvm::object::coff_import_header;
 using llvm::object::coff_section;
 
+class ArchiveFile;
 class Chunk;
 class Defined;
 class DefinedImportData;
 class DefinedImportThunk;
 class Lazy;
 class SectionChunk;
 class Symbol;
 class Undefined;
public:
   // Reads a file (the constructor doesn't do that).
   virtual void parse() = 0;
 
   // Returns the CPU type this file was compiled to.
   virtual MachineTypes getMachineType() { return IMAGE_FILE_MACHINE_UNKNOWN; }
 
   MemoryBufferRef MB;
 
-  // An archive file name if this file is created from an archive.
-  StringRef ParentName;
+  // An archive file if this file is created from an archive.
+  ArchiveFile *Parent = nullptr;
 
   // Returns .drectve section contents if exist.
   StringRef getDirectives() { return StringRef(Directives).trim(); }
 
 protected:
   InputFile(Kind K, MemoryBufferRef M) : MB(M), FileKind(K) {}
 
   std::string Directives;

lld/COFF/InputFiles.cpp

void ArchiveFile::addMember(const Archive::Symbol *Sym) {
   const Archive::Child &C =
       CHECK(Sym->getMember(),
             "could not get the member for symbol " + Sym->getName());
 
   // Return an empty buffer if we have already returned the same buffer.
   if (!Seen.insert(C.getChildOffset()).second)
     return;
 
-  Driver->enqueueArchiveMember(C, Sym->getName(), getName());
+  Driver->enqueueArchiveMember(C, Sym->getName(), this);
 }
 
 std::vector<MemoryBufferRef> getArchiveMembers(Archive *File) {
   std::vector<MemoryBufferRef> V;
   Error Err = Error::success();
   for (const ErrorOr<Archive::Child> &COrErr : File->children(Err)) {
     Archive::Child C =
         CHECK(COrErr,
void ImportFile::parse() {
   // If type is function, we need to create a thunk which jump to an
   // address pointed by the __imp_ symbol. (This allows you to call
   // DLL functions just like regular non-DLL functions.)
   if (Hdr->getType() == llvm::COFF::IMPORT_CODE)
     ThunkSym = Symtab->addImportThunk(Name, ImpSym, Hdr->Machine);
 }
 
 void BitcodeFile::parse() {
+  StringRef ParentName;
+  if (Parent)
+    ParentName = Parent->getName();
   Obj = check(lto::InputFile::create(MemoryBufferRef(
       MB.getBuffer(), Saver.save(ParentName + MB.getBufferIdentifier()))));
   std::vector<std::pair<Symbol *, bool>> Comdat(Obj->getComdatTable().size());
   for (size_t I = 0; I != Obj->getComdatTable().size(); ++I)
     Comdat[I] = Symtab->addComdat(this, Saver.save(Obj->getComdatTable()[I]));
   for (const lto::InputFile::Symbol &ObjSym : Obj->symbols()) {
     StringRef SymName = Saver.save(ObjSym.getName());
     int ComdatIndex = ObjSym.getComdatIndex();
if (Pos == StringRef::npos)
     return Path;
   return Path.substr(Pos + 1);
 }
 
 // Returns a string in the format of "foo.obj" or "foo.obj(bar.lib)".
 std::string lld::toString(const coff::InputFile *File) {
   if (!File)
     return "<internal>";
-  if (File->ParentName.empty())
+  if (!File->Parent)
     return File->getName();
 
-  return (getBasename(File->ParentName) + "(" + getBasename(File->getName()) +
+  return (getBasename(File->Parent->getName()) + "(" + getBasename(File->getName()) +
           ")")
       .str();
 }

lld/COFF/MinGW.cpp

bool AutoExporter::shouldExport(Defined *Sym) const {
   if (Symtab->find(("__imp_" + Sym->getName()).str()))
     return false;
 
   // Check that file is non-null before dereferencing it, symbols not
   // originating in regular object files probably shouldn't be exported.
   if (!Sym->getFile())
     return false;
 
-  StringRef LibName = sys::path::filename(Sym->getFile()->ParentName);
+  if (Sym->getFile()->Parent) {
+    StringRef LibName = sys::path::filename(Sym->getFile()->Parent->getName());
 
     // Drop the file extension.
     LibName = LibName.substr(0, LibName.rfind('.'));
     if (!LibName.empty())
       return !ExcludeLibs.count(LibName);
+  }
 
   StringRef FileName = sys::path::filename(Sym->getFile()->getName());
   return !ExcludeObjects.count(FileName);
 }
 
 void coff::writeDefFile(StringRef Name) {
   std::error_code EC;
   raw_fd_ostream OS(Name, EC, sys::fs::F_None);

lld/COFF/PDB.cpp

Expected<const CVIndexMap&> PDBLinker::maybeMergeTypeServerPDB(ObjFile *File,
 
   // Check for a PDB at:
   // 1. The given file path
   // 2. Next to the object file or archive file
   auto ExpectedSession = tryToLoadPDB(TSId, TSPath);
   if (!ExpectedSession) {
     consumeError(ExpectedSession.takeError());
     StringRef LocalPath =
-        !File->ParentName.empty() ? File->ParentName : File->getName();
+        File->Parent ? File->Parent->getName() : File->getName();
     SmallString<128> Path = sys::path::parent_path(LocalPath);
     sys::path::append(
         Path, sys::path::filename(TSPath, sys::path::Style::windows));
     ExpectedSession = tryToLoadPDB(TSId, Path);
   }
   if (auto E = ExpectedSession.takeError()) {
     TypeServerIndexMappings.erase(TSId);
     MissingTypeServerPDBs.emplace(TSId);
static pdb::SectionContrib createSectionContrib(const Chunk *C, uint32_t Modi) {
   return SC;
 }
 
 void PDBLinker::addObjFile(ObjFile *File) {
   // Add a module descriptor for every object file. We need to put an absolute
   // path to the object into the PDB. If this is a plain object, we make its
   // path absolute. If it's an object in an archive, we make the archive path
   // absolute.
-  bool InArchive = !File->ParentName.empty();
-  SmallString<128> Path = InArchive ? File->ParentName : File->getName();
+  bool InArchive = File->Parent;
+  SmallString<128> Path = InArchive ? File->Parent->getName() : File->getName();
   sys::fs::make_absolute(Path);
   sys::path::native(Path, sys::path::Style::windows);
   StringRef Name = InArchive ? File->getName() : StringRef(Path);
 
   File->ModuleDBI = &ExitOnErr(Builder.getDbiBuilder().addModuleInfo(Name));
   File->ModuleDBI->setObjFileName(Path);
 
   auto Chunks = File->getChunks();

lld/test/COFF/pdb-comdat.test

 CHECK-NEXT:        84 | S_PROCREF [size = 20] `main`
 CHECK-NEXT:             module = 1, sum name = 0, offset = 120
 CHECK-NEXT:       148 | S_PROCREF [size = 20] `bar`
 CHECK-NEXT:             module = 2, sum name = 0, offset = 120
 CHECK-NEXT:       128 | S_PROCREF [size = 20] `foo`
 CHECK-NEXT:             module = 1, sum name = 0, offset = 208
 CHECK-NEXT:       104 | S_GDATA32 [size = 24] `global`
 CHECK-NEXT:             type = 0x0074 (int), addr = 0000:0000
-CHECK-NEXT:       168 | S_GDATA32 [size = 24] `global`
-CHECK-NEXT:             type = 0x0074 (int), addr = 0000:0000
 
 CHECK:                           Symbols
 CHECK: ============================================================
 CHECK-LABEL:   Mod 0000 | `{{.*}}pdb_comdat_main.obj`:
 CHECK:     4 | S_OBJNAME [size = 56] sig=0, `C:\src\llvm-project\build\pdb_comdat_main.obj`
 CHECK:    60 | S_COMPILE3 [size = 60]
 CHECK:         machine = intel x86-x64, Ver = Microsoft (R) Optimizing Compiler, language = c
 CHECK:         frontend = 19.0.24215.1, backend = 19.0.24215.1