This is an archive of the discontinued LLVM Phabricator instance.

[codeview] Emit vftable records
AbandonedPublic

Authored by rnk on Jul 27 2016, 1:02 PM.

Details

Summary

Works towards fixing PR28150

Adds a custom DWARF tag so that we can represent this information in our
DI metadata. VFTable information appears in the elements list of
complete DICompositeTypes and looks like this:

DIDerivedType(
  tag: DW_TAG_LLVM_vftable,
  scope: !<CompositeType>,
  baseType: !<OverriddenVFTable>,
  offset: <VFPtrOffset>,
  name: <VFTableName>,
  extraData: !<MethodList>,
  flags: DIFlagArtificial)

The OverriddenVFTable slot references the vftable from the base class.

Diff Detail

Event Timeline

rnk updated this revision to Diff 65790.Jul 27 2016, 1:02 PM
rnk retitled this revision from to [codeview] Emit vftable records.
rnk updated this object.
rnk added reviewers: dblaikie, aprantl, dexonsmith.
rnk added a subscriber: llvm-commits.
majnemer added inline comments.
test/DebugInfo/COFF/types-data-members.ll
389–422

Might be good to use FileCheck regexes here so that we don't have to update quite so much in the future.

amccarth added inline comments.Jul 28 2016, 11:58 AM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2096

Just a rant about this pattern ... I don't see an easy way to fix this and be consistent with existing code.

The comment says this loop _emits_ stuff, but the loop looks like it just looks up type indices and drops them on the floor. In reality, we're calling this "get..." function for its side effects. The function name doesn't even hint at the side effects. Many people expect methods like "getFoo" to be const accessor methods. And if they're not const, it's probably because there's a little caching involved. But the side effects here involve more than caching.

This seems to be a pervasive anti-pattern in the bits of LLVM I've seen, a pattern that makes it hard (for me, at least) to understand the code. Without that comment, I'd be lost. With it, I at least know I have to go study the guts of what looks like an accessor method to figure out the side effect. It's especially confusing, since many methods have verbs like emit in them.

Anyway, just a rant. I don't expect you to make any changes.

rnk added inline comments.Jul 28 2016, 12:05 PM
lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
2096

Actually, it might be worth rewriting this code a bit. The vftable records refer to each other, so we do need some kind of map from metadata pointer to type index. However, they form a DAG, so we could topologically sort the vftables and then emit them in order and be confident that we will emit a vftable before it is referenced. I was just lazy and used memoized recursion instead, but a topological sort would make the output more understandable.

test/DebugInfo/COFF/types-data-members.ll
389–422

I could go either way, I liked having one "total output" golden test that exercises a variety of stuff.

rnk added inline comments.Aug 30 2016, 12:57 PM
test/DebugInfo/COFF/vftables.ll
45

I asked Microsoft about these records, and it turns out that they are completely unused for debugging. They only exist to power link-time devirtualization. In fact, they are never included in the final linked PDB.

So, I'm going to revert most of this patch, and do something more minimal to get the VFPtr records in the field list.

rnk abandoned this revision.Aug 31 2016, 10:21 AM

Superseded by rL280254, which was much less involved.