Page MenuHomePhabricator

[PDB] Emit old fpo data to the PDB
ClosedPublic

Authored by zturner on Sep 11 2018, 4:30 PM.

Details

Summary

D51951 tried to fix https://bugs.llvm.org/show_bug.cgi?id=38847 by emitting the new fpo data stream to the PDB file. While I confirmed that that did indeed fix one known repro of this symptom, it turns out it didn't fix the one in the reporting bug.

The reason is that FPO data is emitted in two different places into the PDB, each with its own different format. The DBI stream contains a "new fpo" data stream and an "fpo" data stream (which I refer to as the "old" fpo stream). The New Fpo data stream is populated via the contents of the individual object files' DEBUG_S_FRAMEDATA subsections that appear inside of .debug$S sections. This is what modern compilers (both clang-cl and cl) emit. However, the bug in the report is about what happens if a crash occurs inside of memcpy. memcpy is written in assembler, and if you disassemble memcpy.obj from the CRT you find that it does not have a DEBUG_S_FRAMEDATA subsection in its object file. Instead, it contains a .debug$F section, which is something that until now there has not been a single reference to in the entire LLVM, Clang, or LLD codebase. However, some investigation into this shows that it is a trivial format, and is simply a list of FPO_DATA records. Furthermore, this section contains relocations, so unlike the manual relocating we needed to do with the DEBUG_S_FRAMEDATA contents, we just need to apply relocations to this section and copy it through.

But, as mentioned the format is slightly different and it does not go to the new fpo data stream, it goes to its own place.

With this patch I confirmed that the problem in the original bug report no longer reproduces. ChildFunction correctly shows up in my stack trace and I can switch to it and view local variables.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

zturner created this revision.Sep 11 2018, 4:30 PM
rnk added inline comments.Sep 11 2018, 5:12 PM
lld/COFF/PDB.cpp
899–906 ↗(On Diff #164996)

You can check if the section is empty before you relocate it by checking DebugChunk->getSize() == 0. Then you can sink the calls to relocateDebugChunk into the .debug$S handling and only check the section name once.

957 ↗(On Diff #164996)

I would handle .debug$F first and continue early to reduce indentation. It's the less complicated special case.

zturner updated this revision to Diff 165106.Sep 12 2018, 9:52 AM

I might have gone overboard here, but I wanted to kind of put all .debug$S handling into its own function to make the loop simpler and easier to follow. Since resolving .debug$S requires state to be maintained across multiple .debug$S sections in a single object file, I made a class out of it and moved a lot of the local state into the class. .debug$F is simpler and doesn't require state across runs so I just handle that directly in the loop.

rnk added a comment.Sep 12 2018, 1:08 PM

Needs tests? Maybe they just aren't in the diff?

lld/COFF/PDB.cpp
845–846 ↗(On Diff #165106)

I think we can raise this into handleDebugS and then we don't need the HasMagic parameter.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 12 2018, 2:03 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rL342080: [PDB] Emit old fpo data to the PDB file. (authored by zturner, committed by ). · Explain Why
Closed by commit rLLD342080: [PDB] Emit old fpo data to the PDB file. (authored by zturner, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.