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.

Diff Detail

Repository
rL LLVM

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 ↗(On Diff #203108)

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

388 ↗(On Diff #203108)

Maybe:

std::string result = "<missing DWARF unit path>";
392–395 ↗(On Diff #203108)

Maybe create a DWARFUnit function?:

const lldb_private::FileSpec &DWARFUnit::GetFileSpec();
393 ↗(On Diff #203108)

Does this do the right thing if DW_AT_name is absolute?

2266–2267 ↗(On Diff #203108)

Use new DWARFDie::GetFile()

2424–2425 ↗(On Diff #203108)

Use new DWARFDie::GetFile()

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
286–306 ↗(On Diff #203108)

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

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
812–813 ↗(On Diff #203108)

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

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
303 ↗(On Diff #203108)

Can probably just be named GetFile

aprantl added inline comments.Jun 7 2019, 2:22 PM
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
814 ↗(On Diff #203108)

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 ↗(On Diff #203108)

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 ↗(On Diff #203108)

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 ↗(On Diff #203108)

Woops. No, it doesn't.

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
286–306 ↗(On Diff #203108)

Sounds good.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
812–813 ↗(On Diff #203108)

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 ↗(On Diff #203108)

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 ↗(On Diff #203790)

Nit: move this next to GetAbbrevOffset

200 ↗(On Diff #203790)

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

290 ↗(On Diff #203790)

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 ↗(On Diff #203790)

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 ↗(On Diff #203108)

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 ↗(On Diff #203790)

GetAbsolutePath sounds good.

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
821 ↗(On Diff #203790)

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

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
303 ↗(On Diff #203108)

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 ↗(On Diff #203108)

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 ↗(On Diff #203845)

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.

2261 ↗(On Diff #203845)

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

2418 ↗(On Diff #203845)

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

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
289–326 ↗(On Diff #203845)

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 ↗(On Diff #203845)

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