Page MenuHomePhabricator

Implement equal_range for the DWARF v5 accelerator table
ClosedPublic

Authored by labath on Feb 8 2018, 3:46 AM.

Details

Summary

This patch implements the name lookup functionality of the .debug_names
accelerator table and hooks it up to "llvm-dwarfdump -find". To make the
interface of the two kinds of accelerator tables more consistent, I've
created an abstract "DWARFAcceleratorTable::Entry" class, which provides
a consistent interface to access the common functionality of the table
entries (such as getting the die offset, die tag, etc.). I've also
modified the apple table to vend entries conforming to this interface.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Feb 8 2018, 3:46 AM
labath added inline comments.Feb 8 2018, 3:54 AM
tools/llvm-dwarfdump/llvm-dwarfdump.cpp
370 ↗(On Diff #133398)

Technically, this changes behavior for apple tables as previously it would accept any section offset here, but I think the author was looking for the DIE offset here anyway.

This is very neat, thanks a lot Pavel!

A few insignificant nits inline.

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
280 ↗(On Diff #133398)

Nit: Index is usually Idx

284 ↗(On Diff #133398)

... returned Index is valid ...

303 ↗(On Diff #133398)

Missing )

labath updated this revision to Diff 133433.Feb 8 2018, 8:52 AM
labath marked 3 inline comments as done.

Fix issues pointed out by Jonas + clang-format.

dblaikie added inline comments.Feb 8 2018, 10:10 AM
include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
43 ↗(On Diff #133433)

default

290–295 ↗(On Diff #133433)

Could these be defaulted (maybe even implicitly defaulted)?

296 ↗(On Diff #133433)

Perhaps this should/could be implicit?

labath updated this revision to Diff 133580.Feb 9 2018, 3:14 AM
labath marked 2 inline comments as done.

Resolve David's comments (apart from one, which I have a question on).

labath added inline comments.Feb 9 2018, 3:15 AM
include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h
290–295 ↗(On Diff #133433)

These ones are a bit tricky. They could be made default, but then I'd have to un-delete the super class implementations (which I deleted as a standard precaution for polymorphic classes). Maybe I could make the super class functions protected to make sure they're not invoked accidentally ?

(The reason I need these constructors in the first place is so I can return the object as a value from the parsing function without resorting to unique_ptrs and such. I did feel it a bit weird while doing it as this isn't standard practice for polymorphic classes, but it seemed like a nice optimization.)

labath updated this revision to Diff 133828.Feb 12 2018, 3:48 AM
labath marked 2 inline comments as done.

Make the base class copy/move operations protected and remote the hand-coded implementations from the derived class.

dblaikie accepted this revision.Feb 12 2018, 10:30 AM

Also, if possible - the Entry dtor could be made non-virtual and protected in the base class, if these objects are never owned polymorphically, which it /looks/ like they aren't, but not entirely sure.

This revision is now accepted and ready to land.Feb 12 2018, 10:30 AM
JDevlieghere accepted this revision.Feb 12 2018, 11:56 AM
labath updated this revision to Diff 134213.Feb 14 2018, 5:55 AM

Make base Entry destructor protected and nonvirtual (the class is not owned polymorphically right now).

Will commit once issues with the djb hash function are resolved.

This revision was automatically updated to reflect the committed changes.