This is an archive of the discontinued LLVM Phabricator instance.

DWARF: Share line tables of type units
ClosedPublic

Authored by labath on Jun 5 2019, 2:24 AM.

Details

Summary

This patch creates a cache of file lists in line tables referenced by
type units. This cache is used to avoid parsing a line table twice
(since a file list will generally be shared by many type units).

It also sets things up in a way that parsing of DW_AT_decl_file
attributes will keep working even when we stop creating lldb compile
units for dwarf type units, but it stops short of actually doing that.
This means that the request for files now go directly to SymbolFileDWARF
instead of being routed there indirectly via the
lldb_private::CompileUnit class.

As a result of this, a number of occurences of SymbolContext variables
in DWARFASTParserClang have become unused, so I remove them.

This patch reduces the number of times a file list is being parsed, but
the situation is still suboptimal, as the parsed list is being copied
multiple times. This will be fixed when we stop creating CompileUnits
for DWARF type units.

Event Timeline

labath created this revision.Jun 5 2019, 2:24 AM
clayborg added inline comments.Jun 7 2019, 9:55 AM
source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
309

Maybe make a "FileSpec DWARFDie::GetFile(uint32_t idx);" function?

388

Maybe:

std::string result = "<missing DWARF unit path>";
392–395

Maybe create a DWARFUnit function?:

const lldb_private::FileSpec &DWARFUnit::GetFileSpec();
393

Does this do the right thing if DW_AT_name is absolute?

2266–2267

Use new DWARFDie::GetFile()

2424–2425

Use new DWARFDie::GetFile()

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
286–306

Many attributes being individually fetched here. This is slow. We should probably iterate over all attributes?

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
812–813

why are these checks needed? Remove? Maybe left over from previous approach?

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
303

Can probably just be named GetFile

aprantl added inline comments.Jun 7 2019, 2:22 PM
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
814

return {}; ?

labath updated this revision to Diff 203790.Jun 10 2019, 3:05 AM
labath marked 13 inline comments as done.
  • Implement code review suggestions (except the Die::GetName, and return {})
  • Add a test for the error message which is printed when we encounter an incomplete class (this is motivated by the bug in the printing of the name of the compile unit)
source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
309

I'm trying to avoid diverging llvm and lldb DWARFDies more than they already are. As llvm DWARFDie will not be returning a FileSpec, I think going through the symbol file makes sense (that's the level at which the sharing happens anyway.

But even without that, the api doesn't seem right, as this is not a query that can be answered by the DWARFDie class alone.

392–395

Sounds good. This also diverges from llvm DWARFUnit, but OTOH, it fits in nicely with the GetCompilationDirectory function we have there already, so these two can be resolved later at the same time.

393

Woops. No, it doesn't.

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
286–306

Sounds good.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
812–813

The offset is going to be used as a key in a DenseMap, and DenseMap reserves two values of each type for the "empty" and "tombstone" keys. These are something like 0xff..ff and 0xff.ffe, so valid DWARF will not contain those values, but malformed can (and if it did, this code would crash when trying to insert those keys).

814

That wouldn't work, because {} is a temporary, and this function is returning a reference. Returning by reference saves a copy in the common case where DW_AT_stmt_list is valid, but I also need to handle the invalid case somehow. I could have this function return a pointer and return a nullptr in the invalid case (or something equivalent), but this saves the caller from checking that.

JDevlieghere added inline comments.Jun 10 2019, 8:58 AM
source/Plugins/SymbolFile/DWARF/DWARFUnit.h
148

Nit: move this next to GetAbbrevOffset

200

Given the implementation, how about GetAbsolutePath? I think the "FileSpec" of a DWARF unit is a little weird, but feel free to ignore this.

288

Let's make this a Doxygen comment with ///< or even better just /// on the line above it so it doesn't wrap.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
821

Not your responsibility but we should really get rid of this thing as it doesn't make sense anymore in > DWARF5.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
303

Should this return an optional?

labath marked 5 inline comments as done.Jun 10 2019, 9:25 AM
labath added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFUnit.h
200

GetAbsolutePath sounds good.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
821

I'm waiting on your llvm debug_lines patch. :)

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
303

I don't know. On one hand, both the level above this function and the level below use an empty FileSpec do denote failure, so I'd have to convert it explicitly in both places. OTOH, Optional is the direction we want to move in.

I can change it if you want..

labath updated this revision to Diff 203845.Jun 10 2019, 9:25 AM
  • address code review comments
JDevlieghere accepted this revision.Jun 10 2019, 9:30 AM
JDevlieghere added inline comments.
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
303

Yeah it's a bit of a chicken & egg problem. I don't have a strong opinion, so I'm fine with keeping it as is.

This revision is now accepted and ready to land.Jun 10 2019, 9:30 AM
clayborg requested changes to this revision.Jun 10 2019, 10:09 AM

Still want to resolve getting files from a DWARFUnit a bit better. See inlined comment.

source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
309

At the very least we should be asking the unit for the file? That is why I wanted to be able to ask the DWARFDIE for the file because it contains the right unit. If we just put the API on the unit, then we have the chance someone will use the wrong unit for the file. Also, as the DWARF gets fancier as time goes on (DWO, DWZ, etc), the unit might refer to another unit. But at the very least here I would feel better if we ask the DWARFUnit for the file. The GetDWARF() will ask the DWARFUnit in the DIE for the is DWARF file, then we will call the DWARF file class (SymbolFileDWARF or DWARFContext to get a file from the unit by passing a reference? Seems convoluted. Fine not adding the API as a FileSpec if we are trying to keep the API the same as LLVM.

2267

DWARFUnit::GetFile() or what ever solution we come up with from my previous long inlined comment.

2425

DWARFUnit::GetFile() or what ever solution we come up with from my previous long inlined comment.

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
306–343

much better!

This revision now requires changes to proceed.Jun 10 2019, 10:09 AM
labath marked 11 inline comments as done.Jun 11 2019, 2:37 AM
labath added inline comments.
source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
309

I think asking the unit for the file is fine. I'm not sure why llvm does not do that (it does the "get the context, pass the unit to get the line table, fetch the file" dance), but it sounds like a utility function doing this could be added there. Eventually our DWARFUnit will start using DWARFContext instead of SymbolFileDWARF for the other operations, which will make the two approaches pretty similar.

I've kept the function as returning FileSpecs. Returning any other type would be a pessimization, as the values are already parsed as FileSpecs.

labath updated this revision to Diff 203992.Jun 11 2019, 2:39 AM
labath marked an inline comment as done.
  • add DWARFUnit::GetFile helper function
clayborg accepted this revision.Jun 11 2019, 7:06 AM
This revision is now accepted and ready to land.Jun 11 2019, 7:06 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2019, 4:27 AM