COFF: Add type server pdb files to linkrepro tar file.

[pcc]

Add missing REQUIRES line

[zturner]

It's possible I'm missing something here, but I don't see anything specific to linkrepro or TAR files in any of this patch. This leads me to believe /LINKREPRO is doing a full PDB link, including merging types and symbols from all object files. resolving type servers and merging those in as well.

If I understand this feature correctly, /LINKREPRO isn't actually supposed to link anything. It just copies the necessary files to a TAR file and creates a response file so that that response file, combined with the TAR file can then be linked on a different machine.

If my understanding is wrong feel free to correct me, but if it's right, then this seems overly heavyweight. It looks like this is doing a full PDB link but injecting some sort of Writer class that instead of writing to an output executable + PDB file, writes to a TAR file.

When we discussed this on IRC, I thought the solution was just going to find the type server PDB and add it to the tar file, but not actually link or do anything expensive.

[pcc]

In D38977#899088, @zturner wrote:

It's possible I'm missing something here, but I don't see anything specific to linkrepro or TAR files in any of this patch.

The important part of this patch is the call to takeBuffer on line 229 of PDB.cpp. That will cause the argument to be added to the linkrepro if we are creating one. Admittedly it could be a little clearer what that function does, we may want to rename it now that we're starting to use it outside of the driver.

This leads me to believe /LINKREPRO is doing a full PDB link, including merging types and symbols from all object files. resolving type servers and merging those in as well.

If I understand this feature correctly, /LINKREPRO isn't actually supposed to link anything. It just copies the necessary files to a TAR file and creates a response file so that that response file, combined with the TAR file can then be linked on a different machine.

That's not accurate. /linkrepro in LLD is implemented by adding files to the linkrepro tar file as a side effect of opening files during a full link (note that link.exe's implementation of linkrepro also does a full link). I suppose that it may be possible in principle to implement /linkrepro so that it just creates the tar file, but it seems simplest to do things this way to reduce the possibility of divergence between what linkrepro does and what regular links do.

If my understanding is wrong feel free to correct me, but if it's right, then this seems overly heavyweight. It looks like this is doing a full PDB link but injecting some sort of Writer class that instead of writing to an output executable + PDB file, writes to a TAR file.

No, the output files are not written to the tar file, only the inputs.

[zturner]

lld/test/COFF/linkrepro-pdb.test

1 ↗

(On Diff #119217)

I don't believe this test needs requires shell does it? That will basically prevent it from running on Windows.

[pcc]

lld/test/COFF/linkrepro-pdb.test

1 ↗

(On Diff #119217)

I think the linkrepro tests do in fact require not-Windows, perhaps because of path length limitations. I think Rui did try to enable them on Windows at one point, but that change had to be reverted (see rL291527).

[ruiu]

Yeah, some tests failed on some bots due to the maximum path limitation on Windows. /linkrepro actually works just fine, but in order to verify its contents, we have to untar the produced file into a working directory. That sometimes fails because the length of the current working directory + pathnames in the tar could easily exceed 255 characters long on some bots.

[zturner]

In D38977#900140, @ruiu wrote:

Yeah, some tests failed on some bots due to the maximum path limitation on Windows. /linkrepro actually works just fine, but in order to verify its contents, we have to untar the produced file into a working directory. That sometimes fails because the length of the current working directory + pathnames in the tar could easily exceed 255 characters long on some bots.

Would it be sufficient to:

a) Use tar tf to list the contents
b) untar the file you're interested in like this:

RUN: tar xOf repro.tar %t/ts.pdb > %t/repro/ts.pdb
RUN: diff %t/ts.pdb %t/repro/ts.pdb

lld/test/COFF/linkrepro-pdb.test

4–5 ↗

(On Diff #119217)

Should these output files be going somewhere prefixed with %t?

[zturner]

In D38977#900152, @zturner wrote:

In D38977#900140, @ruiu wrote:

Yeah, some tests failed on some bots due to the maximum path limitation on Windows. /linkrepro actually works just fine, but in order to verify its contents, we have to untar the produced file into a working directory. That sometimes fails because the length of the current working directory + pathnames in the tar could easily exceed 255 characters long on some bots.

Would it be sufficient to:

a) Use tar tf to list the contents
b) untar the file you're interested in like this:

RUN: tar xOf repro.tar %t/ts.pdb > %t/repro/ts.pdb
RUN: diff %t/ts.pdb %t/repro/ts.pdb

In case it wasn't clear, the idea behind tar tf was to run a FileCheck to verify the contents of the tar file.

[ruiu]

If not GNU but all tar commands support the 'O' option, then yes, that should suffice.

[pcc]

In D38977#900155, @ruiu wrote:

If not GNU but all tar commands support the 'O' option, then yes, that should suffice.

Seems worth a try. Looks like gnuwin32 and Darwin tar also support it, and so does FreeBSD: https://github.com/freebsd/freebsd/blob/master/contrib/libarchive/tar/bsdtar.c#L528

lld/test/COFF/linkrepro-pdb.test

4–5 ↗

(On Diff #119217)

We do cd %t above, so they'll end up in that directory.

[zturner]

OpenBSD is the only one I can find after a quick search whose tar doesn't support -O. or at the very least, the man page I found doesn't. On the other hand, if 1-2 platforms are going to have to be disabled no matter what because there is no way to write it for all platforms, and we can choose between "enabled on Windows, disabled on platform X" versus "enabled on platform X, disabled on Windows", I think we should prefer the former, since this is in the Windows linker.

[ruiu]

Maybe you want to add gnutar predicate so that you can write REQUIRES: gnutar?

[pcc]

Use "tar xOf" to try to avoid path length limitations on Windows

Depends on D39023

[pcc]

• Add a gnutar feature and start using it for the test

[ruiu]

LGTM

lld/trunk/COFF/Driver.h

public:
 
   // 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);
 
+  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);
 
   // Searches a file from search paths.
   Optional<StringRef> findFile(StringRef Filename);
private:
   // choose the right one depending on which "main" function is defined.
   // This function looks up the symbol table and resolve corresponding
   // entry point name.
   StringRef findDefaultEntry();
   WindowsSubsystem inferSubsystem();
 
   void invokeMSVC(llvm::opt::InputArgList &Args);
 
-  MemoryBufferRef takeBuffer(std::unique_ptr<MemoryBuffer> MB);
   void addBuffer(std::unique_ptr<MemoryBuffer> MB, bool WholeArchive);
   void addArchiveBuffer(MemoryBufferRef MBRef, StringRef SymName,
                         StringRef ParentName);
 
   void enqueuePath(StringRef Path, bool WholeArchive);
 
   void enqueueTask(std::function<void()> Task);
   bool run();

lld/trunk/COFF/PDB.cpp

 //===- PDB.cpp ------------------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "PDB.h"
 #include "Chunks.h"
 #include "Config.h"
+#include "Driver.h"
 #include "Error.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "Writer.h"
 #include "llvm/DebugInfo/CodeView/CVDebugRecord.h"
 #include "llvm/DebugInfo/CodeView/DebugSubsectionRecord.h"
 #include "llvm/DebugInfo/CodeView/LazyRandomTypeCollection.h"
 #include "llvm/DebugInfo/CodeView/RecordName.h"
const CVIndexMap &PDBLinker::mergeDebugT(ObjFile *File,
   if (auto Err = mergeTypeAndIdRecords(IDTable, TypeTable,
                                        ObjectIndexMap.TPIMap, Types))
     fatal(Err, "codeview::mergeTypeAndIdRecords failed");
   return ObjectIndexMap;
 }
 
 static Expected<std::unique_ptr<pdb::NativeSession>>
 tryToLoadPDB(const GUID &GuidFromObj, StringRef TSPath) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr = MemoryBuffer::getFile(
+      TSPath, /*FileSize=*/-1, /*RequiresNullTerminator=*/false);
+  if (!MBOrErr)
+    return errorCodeToError(MBOrErr.getError());
+
   std::unique_ptr<pdb::IPDBSession> ThisSession;
-  if (auto EC =
-          pdb::loadDataForPDB(pdb::PDB_ReaderType::Native, TSPath, ThisSession))
+  if (auto EC = pdb::NativeSession::createFromPdb(
+          MemoryBuffer::getMemBuffer(Driver->takeBuffer(std::move(*MBOrErr)),
+                                     /*RequiresNullTerminator=*/false),
+          ThisSession))
     return std::move(EC);
 
   std::unique_ptr<pdb::NativeSession> NS(
       static_cast<pdb::NativeSession *>(ThisSession.release()));
   pdb::PDBFile &File = NS->getPDBFile();
   auto ExpectedInfo = File.getPDBInfoStream();
   // All PDB Files should have an Info stream.
   if (!ExpectedInfo)

lld/trunk/test/COFF/linkrepro-pdb.test

+REQUIRES: x86, gnutar
+
+RUN: rm -rf %t && mkdir -p %t && cd %t
+RUN: yaml2obj %S/Inputs/pdb-type-server-simple-a.yaml -o a.obj
+RUN: yaml2obj %S/Inputs/pdb-type-server-simple-b.yaml -o b.obj
+RUN: llvm-pdbutil yaml2pdb %S/Inputs/pdb-type-server-simple-ts.yaml -pdb ts.pdb
+RUN: lld-link a.obj b.obj -entry:main -debug -out:t.exe -pdb:t.pdb -nodefaultlib -linkrepro:.
+RUN: tar xOf repro.tar repro/%:t/ts.pdb > repro-ts.pdb
+RUN: diff ts.pdb repro-ts.pdb

lld/trunk/test/lit.cfg.py

 # cvtres, which always accompanies it.  Alternatively, check if we can use
 # libxml2 to merge manifests.
 if (lit.util.which('cvtres', config.environment['PATH'])) or \
         (config.llvm_libxml2_enabled == '1'):
     config.available_features.add('manifest_tool')
 
 if (config.llvm_libxml2_enabled == '1'):
     config.available_features.add('libxml2')
+
+tar_version = subprocess.Popen(
+    ['tar', '--version'], stdout=subprocess.PIPE, env={'LANG': 'C'})
+if 'GNU tar' in tar_version.stdout.read().decode():
+    config.available_features.add('gnutar')
+tar_version.wait()

llvm/trunk/include/llvm/DebugInfo/PDB/Native/NativeSession.h

 class PDBFile;
 
 class NativeSession : public IPDBSession {
 public:
   NativeSession(std::unique_ptr<PDBFile> PdbFile,
                 std::unique_ptr<BumpPtrAllocator> Allocator);
   ~NativeSession() override;
 
-  static Error createFromPdb(StringRef Path,
+  static Error createFromPdb(std::unique_ptr<MemoryBuffer> MB,
                              std::unique_ptr<IPDBSession> &Session);
   static Error createFromExe(StringRef Path,
                              std::unique_ptr<IPDBSession> &Session);
 
   std::unique_ptr<PDBSymbolCompiland>
   createCompilandSymbol(DbiModuleDescriptor MI);
 
   std::unique_ptr<PDBSymbolTypeEnum>

llvm/trunk/lib/DebugInfo/PDB/Native/NativeSession.cpp

 } // namespace
 
 NativeSession::NativeSession(std::unique_ptr<PDBFile> PdbFile,
                              std::unique_ptr<BumpPtrAllocator> Allocator)
     : Pdb(std::move(PdbFile)), Allocator(std::move(Allocator)) {}
 
 NativeSession::~NativeSession() = default;
 
-Error NativeSession::createFromPdb(StringRef Path,
+Error NativeSession::createFromPdb(std::unique_ptr<MemoryBuffer> Buffer,
                                    std::unique_ptr<IPDBSession> &Session) {
-  ErrorOr<std::unique_ptr<MemoryBuffer>> ErrorOrBuffer =
-      MemoryBuffer::getFileOrSTDIN(Path, /*FileSize=*/-1,
-                                   /*RequiresNullTerminator=*/false);
-  if (!ErrorOrBuffer)
-    return make_error<GenericError>(generic_error_code::invalid_path);
-
-  std::unique_ptr<MemoryBuffer> Buffer = std::move(*ErrorOrBuffer);
+  StringRef Path = Buffer->getBufferIdentifier();
   auto Stream = llvm::make_unique<MemoryBufferByteStream>(
       std::move(Buffer), llvm::support::little);
 
   auto Allocator = llvm::make_unique<BumpPtrAllocator>();
   auto File = llvm::make_unique<PDBFile>(Path, std::move(Stream), *Allocator);
   if (auto EC = File->parseFileHeaders())
     return EC;
   if (auto EC = File->parseStreamData())

llvm/trunk/lib/DebugInfo/PDB/PDB.cpp

 #include "llvm/Support/Error.h"
 
 using namespace llvm;
 using namespace llvm::pdb;
 
 Error llvm::pdb::loadDataForPDB(PDB_ReaderType Type, StringRef Path,
                                 std::unique_ptr<IPDBSession> &Session) {
   // Create the correct concrete instance type based on the value of Type.
-  if (Type == PDB_ReaderType::Native)
-    return NativeSession::createFromPdb(Path, Session);
+  if (Type == PDB_ReaderType::Native) {
+    ErrorOr<std::unique_ptr<MemoryBuffer>> ErrorOrBuffer =
+        MemoryBuffer::getFileOrSTDIN(Path, /*FileSize=*/-1,
+                                     /*RequiresNullTerminator=*/false);
+    if (!ErrorOrBuffer)
+      return make_error<GenericError>(generic_error_code::invalid_path, Path);
+
+    return NativeSession::createFromPdb(std::move(*ErrorOrBuffer), Session);
+  }
 
 #if LLVM_ENABLE_DIA_SDK
   return DIASession::createFromPdb(Path, Session);
 #else
   return make_error<GenericError>("DIA is not installed on the system");
 #endif
 }