This is an archive of the discontinued LLVM Phabricator instance.

DebugInfo: Add ability to not emit DW_AT_vtable_elem_location for virtual functions.
ClosedPublic

Authored by pcc on Mar 16 2016, 7:19 PM.

Details

Summary

A virtual index of -1u indicates that the subprogram's virtual index is
unrepresentable (for example, when using the relative vtable ABI), so do
not emit a DW_AT_vtable_elem_location attribute for it.

Diff Detail

Event Timeline

pcc updated this revision to Diff 50904.Mar 16 2016, 7:19 PM
pcc retitled this revision from to DebugInfo: Add ability to not emit DW_AT_vtable_elem_location for virtual functions..
pcc updated this object.
pcc added reviewers: dblaikie, echristo, dexonsmith.
pcc added a subscriber: llvm-commits.
dblaikie added inline comments.Mar 17 2016, 10:47 AM
lib/IR/DIBuilder.cpp
698

Is zero not sufficient in this spot (& the other below) - since we still have it conditional on virtuality anyway?

test/Assembler/disubprogram.ll
34

What's this test testing? I can't quite see what's new/unique/interesting about it.

test/DebugInfo/Generic/virtual-index.ll
46

Given the verbosity of the IR, we usually put all the CHECK lines up above the IR (& include the C++ source that the IR is derived/generate from for illustrative purposes in a comment above the CHECK lines)

pcc updated this revision to Diff 50964.Mar 17 2016, 11:49 AM
pcc marked an inline comment as done.
  • Address review comments
pcc added a comment.Mar 17 2016, 11:49 AM

Duncan wrote:

Judicious use of CHECK-SAME: , ... will let you cherry-pick the interesting fields.

Done.

lib/IR/DIBuilder.cpp
698

Almost, but the IR writer output is not conditional on virtuality, so for example if a client used DIBuilder::createFunction to create a subprogram the IR writer's output for that subprogram would include a zero virtualIndex. Not a major point, but it does make the IR writer output a little easier to match against.

test/Assembler/disubprogram.ll
34

This is testing the changed functionality in the IR writer.

test/DebugInfo/Generic/virtual-index.ll
46

Done

dblaikie accepted this revision.Mar 17 2016, 2:37 PM
dblaikie edited edge metadata.

looks good - one piece of optional feedback (make the IR writer consistent with the output selection)

lib/IR/DIBuilder.cpp
698–700

Should we just make the IR writer conditional on virtuality, then? seems oddly inconsistent this way?

This revision is now accepted and ready to land.Mar 17 2016, 2:37 PM
pcc added inline comments.Mar 17 2016, 2:45 PM
lib/IR/DIBuilder.cpp
698–700

I don't have a strong opinion on the matter, but I'd normally favour simplicity in the IR writer. But since the bitcode writer writes out these virtual indices (even for non-virtual functions) in VBR format and -1u has a longer representation than 0, I think that tips the scales in favour of using 0 here and adding the conditional to the IR writer. I'll do that then.

This revision was automatically updated to reflect the committed changes.