This is an archive of the discontinued LLVM Phabricator instance.

[PDB] Error on too large stream directories
ClosedPublic

Authored by hans on Feb 20 2023, 5:32 AM.

Details

Summary

We hit this in Chromium builds where the PDB file was just under 4GB, but the stream directory was actually too large to be correctly represented.

llvm-pdbutil would error about this in llvm::msf::validateSuperBlock, but lld should not write such PDB files in the first place.

Diff Detail

Event Timeline

hans created this revision.Feb 20 2023, 5:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2023, 5:32 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
hans requested review of this revision.Feb 20 2023, 5:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2023, 5:32 AM

In addition to these changes, do you think you can add a more descriptive error at a higher level in LLD, maybe in PDBLinker::commit(). It is not clear from the message what the corrective measure is. I suppose adding /pdbpagesize: with a higher block size would fix the problem, but this isn't obvious to everyone.

hans updated this revision to Diff 499504.Feb 22 2023, 7:24 AM

In addition to these changes, do you think you can add a more descriptive error at a higher level in LLD, maybe in PDBLinker::commit(). It is not clear from the message what the corrective measure is. I suppose adding /pdbpagesize: with a higher block size would fix the problem, but this isn't obvious to everyone.

I've updated the patch. Please take another look.

aganea accepted this revision.Feb 22 2023, 10:58 AM

Thanks for the changes!

I'm seeing that Zach added quite a few tests for MSF, not sure if you had the time/if it's worth crafting a test for this.

Otherwise, looks good.

lld/COFF/PDB.cpp
1695 ↗(On Diff #499504)

Just a nit: How does the errors look like in stderr? I wonder if we shouldn't use joinErrors to have both errors contiguous on a single line?

llvm/lib/DebugInfo/MSF/MSFError.cpp
47

s/Stream/stream/ to be consistent with the other messages.

This revision is now accepted and ready to land.Feb 22 2023, 10:58 AM
hans marked 2 inline comments as done.Feb 24 2023, 6:11 AM

I'm seeing that Zach added quite a few tests for MSF, not sure if you had the time/if it's worth crafting a test for this.

Thanks! I had a look, and it doesn't seem that those hit MSFBuilder::commit, and creating one is probably not worth the effort for this check.

lld/COFF/PDB.cpp
1695 ↗(On Diff #499504)

It currently looks like this:

lld-link: error: PDB Stream directory too large. The directory block map (4148 bytes) doesn't fit in a block (4096 bytes)
lld-link: error: try using a larger /pdbpagesize:
lld-link: error: failed to write PDB file ./chrome.dll.pdb

I see your point about joining them, but I think with the separate line it's easier to read.

This revision was landed with ongoing or failed builds.Feb 24 2023, 6:12 AM
This revision was automatically updated to reflect the committed changes.
hans marked an inline comment as done.