This is an archive of the discontinued LLVM Phabricator instance.

[LLD][PDB] Implement FIXME: Warn on missing TypeServer PDB rather than error
ClosedPublic

Authored by colden on Jan 17 2018, 10:44 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

colden created this revision.Jan 17 2018, 10:44 AM
rnk requested changes to this revision.Jan 17 2018, 11:14 AM

I think the right approach is to propagate failure out of mergeDebugT so that we can issue the warning there and then return early from PDBLinker::addObjFile. That implements the suggested recovery in the best way possible: linking the object as if it had no debug info.

This is the code in question:

// Before we can process symbol substreams from .debug$S, we need to process
// type information, file checksums, and the string table.  Add type info to
// the PDB first, so that we can get the map from object file type and item
// indices to PDB type and item indices.
CVIndexMap ObjectIndexMap;
const CVIndexMap &IndexMap = mergeDebugT(File, ObjectIndexMap); // --- Make this Expected or Optional, check and return early

// Now do all live .debug$S sections.
DebugStringTableSubsectionRef CVStrTab;
DebugChecksumsSubsectionRef Checksums;
std::vector<ulittle32_t *> StringTableReferences;
for (SectionChunk *DebugChunk : File->getDebugChunks()) {
This revision now requires changes to proceed.Jan 17 2018, 11:14 AM

Ok, that makes sense.

I'm 99% done implementing the change, but what I'm seeing is when a lib is missing it's PDB, every single obj inside of it tosses the warning. Is this the behavior we want? Or is there another way to make sure the warning only goes once per file?

With the warning inside PDBLinker::maybeMergeTypeServerPDB, it just warns the first time the file load is attempted.

rnk added a comment.Jan 17 2018, 1:14 PM

Ok, that makes sense.

I'm 99% done implementing the change, but what I'm seeing is when a lib is missing it's PDB, every single obj inside of it tosses the warning. Is this the behavior we want? Or is there another way to make sure the warning only goes once per file?

With the warning inside PDBLinker::maybeMergeTypeServerPDB, it just warns the first time the file load is attempted.

I think MSVC will also warn once for every object. I think it's pretty reasonable, this isn't something you want to ignore for long.

Works for me! Update incoming.

colden updated this revision to Diff 130260.Jan 17 2018, 1:18 PM

Changes requested by @rnk. Warning is now handled by addObjFile().

rnk added inline comments.Jan 17 2018, 1:36 PM
COFF/PDB.cpp
768–769 ↗(On Diff #130260)

This is the MSVC warning text:

t.obj : warning LNK4099: PDB 'vc140.pdb' was not found with 't.obj' or at 'C:\src\llvm-project\build\vc140.pdb'; linking object as if no debug info

I think it might be worth including the .obj path or at least the basename and mentioning that we're ignoring its debug info. Maybe something like:

warning: Type server PDB foo.pdb was not found, ignoring debug info from foo.obj
colden updated this revision to Diff 130266.Jan 17 2018, 1:53 PM

Got as close as I could to @rnk's suggested error format, while still using toString(theError).

New error example:
warning: Type server PDB for tmp32.dbg\dso_win32.obj is invalid, ignoring debug info. PDB Error: Type server PDB was not found. F:\external-libs\openssl\openssl-1.0.2k\tmp32.dbg\lib.pdb

rnk accepted this revision.Jan 17 2018, 1:55 PM

Looks good, thanks!

This revision is now accepted and ready to land.Jan 17 2018, 1:55 PM

Yay, thanks! This is one of the last bugs that's blocking me. At some point I'll have to figure out how to get these changes merged to 6.0...

In the mean time, would you mind submitting this to trunk for me?

colden updated this revision to Diff 130301.Jan 17 2018, 3:38 PM

Rebased on top of the timer change

Hey guys, is there anything holding this back? If not, I'd love it if one of you could submit it for me.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.