This is an archive of the discontinued LLVM Phabricator instance.

[lld] Correctly link S_FILESTATIC records
ClosedPublic

Authored by zturner on Jan 3 2018, 2:39 PM.

Details

Summary

S_FILESTATIC records are relatively rare, but cl does generate them. In particular, it seems to only generate them with optimizations turned on and a file static is en-registered into a local variable. The key piece of information in an S_FILESTATIC record that is not present in, for example, an S_LOCAL register, is the file (i.e. module) name that the file static comes from.

The prevailing theory is that this is so that if a function in module a which uses a file static defined in module a is then inlined into some other module b, there is a path back to the original S_LDATA32 record.

In any case, although clang does not generate S_FILESTATIC records, it needs to be able to link them. This was broken before, because S_FILESTATIC records contain a field which refers to the string table. However, unlike other records which refer to the string table (but do so indirectly through the File Checksums table), S_FILESTATIC records point directly into the string table.

To handle this, during linking we need to merge in all strings from S_FILESTATIC records into the master PDB string table, and fixup their offsets in the linked S_FILESTATIC record.

This patch implements this.

This issue was originally discovered by Alexander Ganea as it was causing Visual Studio to crash.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 128569.Jan 3 2018, 2:39 PM
zturner created this revision.

Remove some debugging code.

zturner updated this revision to Diff 128570.Jan 3 2018, 2:43 PM

Remove a debugbreak and change it to a log message that appears only with /verbose.

rnk added inline comments.Jan 3 2018, 2:48 PM
lld/COFF/PDB.cpp
727–728 ↗(On Diff #128568)

Unused?

lld/test/COFF/Inputs/pdb-file-statics-a.yaml
13 ↗(On Diff #128568)

We can delete the SectionData for .debug$S sections.

lld/test/COFF/pdb-file-static.yaml
56 ↗(On Diff #128568)

Does this really work? It looks like you deleted all the symbols but you still have relocations against the symbols you deleted.

zturner added inline comments.Jan 3 2018, 2:50 PM
lld/test/COFF/pdb-file-static.yaml
56 ↗(On Diff #128568)

Oops this file is supposed to be gone. Eventually I just decided to check in the entire yaml files in all their glory. They are suffixed with -a and -b. This one was supposed to be removed.

zturner updated this revision to Diff 128572.Jan 3 2018, 3:08 PM

Remove pdb-file-statics.yaml, the real test is called pdb-file-statics.test

rnk added inline comments.Jan 3 2018, 3:25 PM
lld/COFF/PDB.cpp
440–441 ↗(On Diff #128572)

I guess we could implement this, but I take it we've never encountered an object file with these records?

lld/test/COFF/Inputs/pdb-file-statics-a.yaml
43 ↗(On Diff #128572)

I hate, hate, hate this __vc_attributes pollution from C++ debug info test case. Oh well. It's not reasonable to do the work to remove it.

130 ↗(On Diff #128572)

ditto

1299 ↗(On Diff #128572)

ditto

1415 ↗(On Diff #128572)

ditto

13 ↗(On Diff #128568)

still relevant

lld/test/COFF/Inputs/pdb-file-statics-b.yaml
124 ↗(On Diff #128572)

ditto

zturner added inline comments.Jan 3 2018, 4:39 PM
lld/COFF/PDB.cpp
440–441 ↗(On Diff #128572)

Yea, I'd like to figure out how to generate them, perhaps running llvm-pdbutil against a large corpus of object files and PDBs could turn something up. I plan to try it on chrome's PDBs one of these days.

lld/test/COFF/Inputs/pdb-file-statics-a.yaml
43 ↗(On Diff #128572)

Me too. I wish I knew what they were for.

zturner updated this revision to Diff 128582.Jan 3 2018, 4:39 PM

Remove all the SectionData fields from the YAML

rnk accepted this revision.Jan 4 2018, 11:08 AM

lgtm, ship it!

This revision is now accepted and ready to land.Jan 4 2018, 11:08 AM
This revision was automatically updated to reflect the committed changes.