Page MenuHomePhabricator

[LLD][COFF] Early load PDB type server files
Needs ReviewPublic

Authored by aganea on Apr 1 2019, 1:28 PM.

Details

Summary

In order to have all inputs loaded & available upfront when starting to merge types, we need to open PDB type server files early in the process.

PDB type servers are created when using MSVC /Zi. In that case, the debug info for a given project is stored in a single PDB type server file. All OBJ files in that project will refer to their corresponding PDB through a TypeServer2Record.

This patch also fixes a tiny issue where the PDB "Age" wasn't validated against the OBJ's TypeServer2Record.Age. The age is a concept used by MSVC for incremental compilation: the PDB's Age is incremented by MSVC on each incremental build, but the GUID remains the same.

This patch is step 2. in "Proposed commit strategy" in D59226

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

aganea created this revision.Apr 1 2019, 1:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2019, 1:28 PM
aganea edited the summary of this revision. (Show Details)Apr 1 2019, 1:32 PM

Ping! I know Reid is OOO this week and Zachary might not be available for reviewing. @ruiu could you please take a quick look at this? Thank you in advance for your time.

aganea edited the summary of this revision. (Show Details)Apr 17 2019, 6:00 AM
takuto.ikuta added inline comments.Apr 17 2019, 4:40 PM
COFF/InputFiles.cpp
44 ↗(On Diff #193163)

better to not have llvm:: prefix for type in this code when we have this?

722 ↗(On Diff #193163)

const ObjFile *File ?

745 ↗(On Diff #193163)

can be const ObjFile * here too?

758 ↗(On Diff #193163)

same here?

ruiu added a comment.Apr 17 2019, 7:01 PM

InputFile abstracts the concept of a file containing symbols. In each parse() function, we parse a symbol table and then insert symbols to the symbol table. parse() is part of a symbol resolution process step.

Since PDBInputFile doesn't contain any symbols, it doesn't fit to that concept, and it doesn't seem you need to parse them while we parse other types of input files.

So, why don't you just add a pdb file to some global list, and after resolving all symbols, parse them in a simple for loop? I guess that's conceptually much simpler.

rnk added a comment.Apr 18 2019, 11:56 AM

InputFile abstracts the concept of a file containing symbols. In each parse() function, we parse a symbol table and then insert symbols to the symbol table. parse() is part of a symbol resolution process step.

Since PDBInputFile doesn't contain any symbols, it doesn't fit to that concept, and it doesn't seem you need to parse them while we parse other types of input files.

So, why don't you just add a pdb file to some global list, and after resolving all symbols, parse them in a simple for loop? I guess that's conceptually much simpler.

IIUC, the point of this change is to start loading types in parallel as early as possible. The suggestion to accumulate the inputs on the side and wait until after symbol resolution won't exploit all the parallelism that's available.

In D60095#1472111, @rnk wrote:

InputFile abstracts the concept of a file containing symbols. In each parse() function, we parse a symbol table and then insert symbols to the symbol table. parse() is part of a symbol resolution process step.

Since PDBInputFile doesn't contain any symbols, it doesn't fit to that concept, and it doesn't seem you need to parse them while we parse other types of input files.

So, why don't you just add a pdb file to some global list, and after resolving all symbols, parse them in a simple for loop? I guess that's conceptually much simpler.

IIUC, the point of this change is to start loading types in parallel as early as possible. The suggestion to accumulate the inputs on the side and wait until after symbol resolution won't exploit all the parallelism that's available.

As long as this goes in the LinkerDriver's TaskQueue, it's fine, no? I could simply move PDBInputFile out of the InputFile hierarchy, I think that's what @ruiu was proposing?
It wasn't my goal to parallelize the TaskQueue just yet, rather introduce the ghash parallelism first, then parallelize the Type merging.

aganea updated this revision to Diff 196643.Thu, Apr 25, 8:48 AM
aganea marked 4 inline comments as done.
aganea retitled this revision from [LLD][COFF] Move PDB type server loading from PDB.cp early into InputFiles.cpp and introduce PDBInputFile to [LLD][COFF] Early load PDB type server files.
aganea edited the summary of this revision. (Show Details)
aganea added a reviewer: mstorsjo.

I simplfied the patch by moving the PDBInputFile behavior into the existing TypeServerSource class. This makes things more elegant and keeps everything related to debug types into one place.

Tested (check-lld) on three different configurations:

  1. MSBuild + MSVC 15.9.11 with LLVM_ENABLE_ASSERTIONS on, in a Debug target, under Windows 10 1803.
  2. Ninja + Clang 8.0 in a Release target, under Windows 10 1803.
  3. Ninja + GCC 7.3 in a Release target, under WSL/Ubuntu 18.04.
aganea added a comment.Tue, May 7, 6:30 AM

Ping! Any further suggestions? Thanks in advance for your time.

ruiu added a comment.Mon, May 20, 6:52 AM

Overall looking good.

COFF/DebugTypes.cpp
35–57

If a constructor can fail, one pattern to handle that is to make the constructor private and instead provide static ErrorOr<TypeServerSource> getInstance() function. In that function, you can load a PDB and then instantiate TypeServerSouce only when the file loading succeeded. If failed, you can return an error.

144–145

nit: return Path + sys::path::...

aganea updated this revision to Diff 200524.Tue, May 21, 9:14 AM
aganea marked 3 inline comments as done.
aganea added inline comments.Tue, May 21, 9:16 AM
COFF/DebugTypes.cpp
35–57

As suggested, added static Expected<TypeServerSource *> TypeServerSource::getInstance() and replaced previous lld::coff::makeTypeServerSource() by lld::coff::loadTypeServerSource() to be consistent.

Note - PDB load errors are simply queued in TypeServerSource::Instance, and displayed later by TypeServerSource::findFromFile() which is called by PDBLinker::maybeMergeTypeServerPDB().
I've tried displaying the errors on the spot, but at that point we don't know who requested loading of that PDB, and it feels wrong to decorelate the PDB errors from the requesting OBJ:

lld-link: warning: 'bad-block-size.pdb': The PDB file is corrupt. MSF superblock is missing
...
... (various other warnings)
...
lld-link: warning: Cannot use debug info for 'F:\svn\build\tools\lld\test\COFF\Output\pdb-type-server-native-errors.yaml.tmp.obj' [LNK4099]
>>> failed to load reference 'bad-block-size.pdb': (see above)

Given it's not a fatal error, I prefer the current behavior, it feels better from a user perspective:

ld-link: warning: Cannot use debug info for 'F:\svn\build\tools\lld\test\COFF\Output\pdb-type-server-native-errors.yaml.tmp.obj' [LNK4099]
>>> failed to load reference 'bad-block-size.pdb': The PDB file is corrupt. MSF superblock is missing