Page MenuHomePhabricator

[lldb/SymbolFile] Don't parse the whole line table for the support files
ClosedPublic

Authored by JDevlieghere on Wed, Jun 10, 10:17 AM.

Details

Summary

Greg noticed that line table parsing has regressed by 5x in Xcode 11.4.1. Rephrased from our (off list) e-mail conversation:

Prior to my patch of using the LLVM line table parsing code, SymbolFileDWARF::ParseSupportFiles would only parse the line table prologues to get the file list for any files that could be in the line table. If we found the file that someone is setting the breakpoint in in the support files list, we would get a valid index. If we didn't, we would not look any further. So someone sets a breakpoint one "MyFile.cpp:12" and if we find "MyFile.cpp" in the support file list for the compile unit, then and only then would we get the entire line table for that compile unit. Now, no matter what, we always fully parse the line table for all compile units any time any file and line breakpoint is set. This creates a serious problem when debugging a large DWARF in .o file project.

This patch re-instates the old behavior. Unfortunately it means we might end up parsing to prologue twice, but I don't think that outweighs the cost of trying to cache/reuse it.

Diff Detail

Event Timeline

JDevlieghere created this revision.Wed, Jun 10, 10:17 AM
clayborg requested changes to this revision.Wed, Jun 10, 8:41 PM
clayborg added a subscriber: clayborg.

Yes this fixes the regression and actually speeds things up a bit. My results were a speed up of 17% to 30% versus 11.3.1 LLDB.

Just one nit in the way we are getting the DW_AT_stmt_list and not adding in the line table offset in the inline comments and this is good to go.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
894–897

DWARFContext::getLineTableForUnit() in llvm/lib/DebugInfo/DWARF/DWARFContext.cpp grabs the DW_AT_stmt_list like this:

auto Offset = toSectionOffset(UnitDIE.find(DW_AT_stmt_list));
if (!Offset)
  return false; // No line table for this compile unit.

uint64_t stmtOffset = *Offset + U->getLineTableOffset();

Modifying this a bit would be a good idea to make sure we are compatible with all DWARF. Or we can put a function into DWARFUnit that does this correctly and switch DWARFContext::getLineTableForUnit() and our code over to use it to avoid duplicated code.

This revision now requires changes to proceed.Wed, Jun 10, 8:41 PM
JDevlieghere retitled this revision from [lldb/SymbolFile] Don't parse the whole line table for the support files (WIP) to [lldb/SymbolFile] Don't parse the whole line table for the support files.
JDevlieghere edited the summary of this revision. (Show Details)
JDevlieghere edited reviewers, added: labath, jankratochvil; removed: jdoerfert.

Use GetLineTableOffset()

JDevlieghere marked an inline comment as done.Thu, Jun 11, 11:02 AM
clayborg accepted this revision.Thu, Jun 11, 12:33 PM

Nice! Our DWARFUnit::GetLineTableOffset() isn't adding some extra offset to the line table offset that the LLVM parser is adding, but we can do this in a separate patch.

This revision is now accepted and ready to land.Thu, Jun 11, 12:33 PM
labath accepted this revision.Fri, Jun 12, 1:14 AM
labath added a subscriber: teemperor.

If this didn't show up on the lldb benchmark charts (https://teemperor.de/lldb-bench/static.html), it would be nice to add a benchmark for this. What exactly were you measuring here?

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
192–193

It would be good to also add a {0} somewhere to print the actual error we received.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jun 12, 9:47 AM

Hey Jonas, any idea if this could be cherry picked into a 11.5.1 update at any point? The performance is hindering 11.4.1 and 11.5 clients at Facebook.