This is an archive of the discontinued LLVM Phabricator instance.

[codeview] support emitting indirect virtual base class information
ClosedPublic

Authored by inglorion on Oct 13 2016, 3:00 PM.

Details

Summary

Fixes PR28281.

MSVC lists indirect virtual base classes in the field list of a class,
using LF_IVBCLASS records. This change makes LLVM emit such records
when processing DW_TAG_inheritance tags with the DIFlagVirtual and
(newly introduced) DIFlagIndirect tags.

Diff Detail

Repository
rL LLVM

Event Timeline

inglorion updated this revision to Diff 74588.Oct 13 2016, 3:00 PM
inglorion retitled this revision from to [codeview] support emitting indirect virtual base class information.
inglorion updated this object.
inglorion added reviewers: rnk, ruiu, zturner.
rnk added inline comments.
include/llvm/DebugInfo/CodeView/TypeRecord.h
1197 ↗(On Diff #74588)

Can you remove this ctor? We appear to use the wrong kind in VirtualBaseClassRecord::deserialize.

include/llvm/IR/DebugInfoFlags.def
44 ↗(On Diff #74588)

@aprantl, should we sorry about running out of these bits now? We only want to use this flag with DW_TAG_inheritance types, so we could do something like reuse the StaticMember flag. We can still introduce a new name, and maybe make it hyper-specific, like IndirectVirtualBase.

This is worth thinking about since these flags go into bitcode, and we need to be backwards compatible with that once we release it.

test/DebugInfo/COFF/inheritance.ll
102 ↗(On Diff #74588)

Interesting, it looks like something was able to optimize D's initializer when you regenerated the test. Seems fine.

zturner added inline comments.Oct 13 2016, 4:08 PM
include/llvm/DebugInfo/CodeView/TypeRecord.h
1197 ↗(On Diff #74588)

Remove which constructor? deserialize still needs to call this constructor right?

test/DebugInfo/COFF/inheritance.ll
33 ↗(On Diff #74588)

What does the {{.*}} syntax mean here?

inglorion added inline comments.Oct 14 2016, 11:31 AM
include/llvm/DebugInfo/CodeView/TypeRecord.h
1197 ↗(On Diff #74588)

I think @rnk wants me to remove the constructor that does not take a Kind parameter, forcing callers to always specify if they want a VirtualBaseClass or an IndirectVirtualBaseClass. Makes sense, and I have updated the code to do that, which I'll send in the next version of this diff.

test/DebugInfo/COFF/inheritance.ll
33 ↗(On Diff #74588)

The {{ and }} are used for regular expression matching. So this matches any sequence of characters enclosed by parentheses. It's the same matching we use in other clauses in the same file, where there needs to be something there, but we don't care about the exact value.

inglorion marked 2 inline comments as done.Oct 14 2016, 12:52 PM
inglorion updated this revision to Diff 74732.EditedOct 14 2016, 12:54 PM

Removed the old VirtualBaseClassRecord constructor, as @rnk suggested.

rnk added inline comments.Oct 19 2016, 4:41 PM
include/llvm/IR/DebugInfoFlags.def
44 ↗(On Diff #74588)

Let's do this. We can reuse the FwdDecl flag as discussed. You can't forward declare a DW_TAG_inheritance node.

aprantl edited edge metadata.Oct 19 2016, 6:58 PM

Sorry, I missed this when it came up originally.
Reusing an otherwise not applicable bit seems fine to me.

inglorion updated this revision to Diff 75366.Oct 20 2016, 4:38 PM
inglorion edited edge metadata.

Made DIFlagIndirectVirtualBase == DIFlagVirtual | DIFlagFwdDecl, so that we don't need to allocate another bit.

rnk accepted this revision.Oct 24 2016, 4:50 PM
rnk edited edge metadata.

lgtm

Sorry for the delay, I thought I stamped this already.

This revision is now accepted and ready to land.Oct 24 2016, 4:50 PM
This revision was automatically updated to reflect the committed changes.