This is an archive of the discontinued LLVM Phabricator instance.

DWZ 03/06: Main functionality
AbandonedPublic

Authored by jankratochvil on Nov 26 2017, 5:18 AM.

Details

Summary

This is the kitchen sink. It also covers separate DWZ common files.

Patch is using (it contains the implementation of) DWARFDataExtractor::OffsetData from D32167 which is not yet accepted and where I agree with the review by Pavel Labath.

All DWZ patches are also applied in: git clone -b dwz git://git.jankratochvil.net/lldb

Diff Detail

Repository
rLLDB LLDB

Event Timeline

jankratochvil created this revision.Nov 26 2017, 5:18 AM
labath edited edge metadata.Nov 27 2017, 2:31 AM

I am only commenting on the c++14 stuff. I think all of the things you need here are available to us already through various llvm utilities. See inline comments for details.

For the "meat" of this patchset, I think clayborg is the only one who can do a proper review of that.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
74–75

Is llvm::sys::RWMutex what you need here?

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

Have you looked at llvm::DenseSet? It already supports heterogenous lookup( find_as(...)). It can also be more efficient than unordered_set.

487

Will std::atomic<size_t> not work?

Going to update the patch.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h
74–75

Yes, llvm::sys::RWMutex should do the trick, thanks.

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

llvm::DenseSet should do the trick, thanks.

487

Yes, std::atomic_size_t would work but it currently has no benefit (rather a needless overhead) there without read/write locks, the atomicity makes sense if it is read-locked only. With llvm::sys::RWMutex now I will use the atomic type.

jankratochvil marked 2 inline comments as done.Nov 30 2017, 6:17 AM
jankratochvil added inline comments.
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
469

"The iterators in a DenseMap are invalidated whenever an insertion occurs, unlike map." - there is now std::unordered_set<DWZCommonFile, ...> and a pointer to it by DWZCommonFile *m_dwz_common_file;. So thanks for the hint but I do not find DenseSet to be applicable here.

jankratochvil marked an inline comment as done.
jankratochvil retitled this revision from DWZ 11/12: Main functionality to DWZ 09/11: Main functionality.

ping: Are there more comments how to make this whole patchset accepted upstream? Thanks.

Too many changes to give the whole thing the OK to checkin. I will worry about each one as an individual patch. 02/11 next.

I think the most controversial issue here is the testing... davide may have some thoughts on that.

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

the references are superfluous here (this just caught my eye because it's at the bottom of the patch).

jankratochvil marked an inline comment as done.Mar 18 2018, 11:45 AM
jankratochvil planned changes to this revision.Apr 14 2018, 4:22 AM
jankratochvil retitled this revision from DWZ 09/11: Main functionality to DWZ 05/07: Main functionality.

It is now reworked without FileOffset and the remapping to unique DIE offsets for each DW_TAG_partial_unit inclusion by DW_TAG_imported_unit, as discussed in: https://lists.llvm.org/pipermail/lldb-commits/Week-of-Mon-20180409/040324.html

jankratochvil planned changes to this revision.May 13 2018, 12:43 PM
jankratochvil retitled this revision from DWZ 05/07: Main functionality to DWZ 03/06: Main functionality.
jankratochvil edited the summary of this revision. (Show Details)Jul 29 2018, 1:50 PM
jankratochvil set the repository for this revision to rLLDB LLDB.Aug 2 2018, 9:25 AM
jankratochvil added a project: Restricted Project.Aug 30 2018, 7:14 AM
jankratochvil planned changes to this revision.Jun 27 2019, 3:45 AM
jankratochvil abandoned this revision.Feb 7 2021, 9:58 PM

Replaced by D96240.