This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Generalize DwarfAccelTable
AbandonedPublic

Authored by JDevlieghere on Jan 18 2018, 8:57 AM.

Details

Summary

This patch is an effort to make the DwarfAccelTable more generic so they
can be reused by the dsymutil implementation, where we need to support
different atoms for LLDB:

eAtomTypeTypeFlags = 5u,    // Flags from enum TypeFlags
eAtomTypeQualNameHash = 6u  // A 32 bit hash of the full qualified name

Similar to some patches from Fred in the past, my goal was to make the
DwarfAccelTable generic and the emitted data is actually derived from
the list of atoms passed to its constructor. For this purpose I have
added a callback function to the interface. This callback function can
is used to generate a value for a combination of row (HashDataContents)
and column (Atom).

I believe this approach provides the best trade-off between making the
table more generic while still building on the current, proven
implementation.

As discussed on the mailing list
(http://lists.llvm.org/pipermail/llvm-dev/2018-January/120499.html) we
want to bring DWARFv5 accelerator tables to LLVM. The goal is to share
as much logic as possible between the two implementation. With that in
mind, I believe it's beneficial to have llvm-dsymutil use the current
implementation, as to ensure that the future design supports this use
case down the line.

From the emitted code point of view, the patch is actually NFC, we rely
on the existing tests to prove that the patch doesn't break anything.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Jan 18 2018, 8:57 AM

As an alternative to having a callback function it should be trivial to implement this using the a separate DwarfAccelTableInfo class with virtual methods as is common in LLVM. However the current approach requires less code to be changed and follows the same principle.

aprantl added inline comments.Jan 18 2018, 9:28 AM
lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp
35

Could this be merged with DIEInteger::SizeOf() ?

lib/CodeGen/AsmPrinter/DwarfAccelTable.h
169

Is this code not needed for dwarfdump?

188
  default: return 0;
  }
}

not sure if actually better :-)

242

Would be nice to doxygenify these comments in a separate NFC commit. (no need for a review)

labath added inline comments.Jan 18 2018, 9:31 AM
lib/CodeGen/AsmPrinter/DwarfAccelTable.h
163

Do we need the HashDataContents struct if it is just a DIE* wrapper? I am not sure if there is anything that we would want to emit that cannot extracted from a DIE pointer by a suitable DataForAtomFunction callback...

JDevlieghere marked 4 inline comments as done.Jan 18 2018, 9:46 AM
JDevlieghere added inline comments.
lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp
35

I didn't know about this, sounds like a good idea.

lib/CodeGen/AsmPrinter/DwarfAccelTable.h
163

The struct will become useful when I implement the second part for dsymutil. There we don't have DIEs and want to track just the offsets.

169

What do you mean exactly?

188

From experience I know that gcc used to complain about this with -Wall. I kind of prefer this but I don't have a strong opinion.

242

Indeed. I actually have this commit ready :-)

aprantl added inline comments.Jan 18 2018, 9:55 AM
lib/CodeGen/AsmPrinter/DwarfAccelTable.h
169

The print function is asserts (!NDEBUG) only; I was wondering if we don't want to print this in llvm-dwarfdump ever.

JDevlieghere marked 6 inline comments as done.Jan 18 2018, 10:02 AM
JDevlieghere added inline comments.
lib/CodeGen/AsmPrinter/DwarfAccelTable.h
169

Right. This code doesn't actually read any data so I don't think it would benefit dwarfdump unless we manage to unify this with its counterpart in DebugInfo.

labath added inline comments.Jan 18 2018, 10:02 AM
lib/CodeGen/AsmPrinter/DwarfAccelTable.h
169

llvm-dwarfdump uses DWARFAcceleratorTable class (which is in no relation to DwarfAccelTable) :P.

JDevlieghere marked an inline comment as done.

Use DIEInteger::SizeOf()

JDevlieghere abandoned this revision.Jan 23 2018, 3:59 AM
JDevlieghere marked 2 inline comments as done.

Abandoning this in favor of D42334