Page MenuHomePhabricator

Let replaceVTableHolder accept any type
ClosedPublic

Authored by tromey on Nov 1 2017, 1:05 PM.

Details

Summary

In Rust, a trait can be implemented for any type, and if a trait
object pointer is used for the type, then a virtual table will be
emitted for that trait/type combination.

We would like debuggers to be able to inspect trait objects, which
requires finding the concrete type associated with a given vtable.

This patch changes LLVM so that any type can be passed to
replaceVTableHolder. This allows the Rust compiler to emit the needed
debug info -- associating a vtable with the concrete type for which it
was emitted.

This is a DWARF extension: DWARF only specifies the meaning of
DW_AT_containing_type in one specific situation. This style of DWARF
extension is routine, though, and LLVM already has one such case for
DW_AT_containing_type.

Diff Detail

Repository
rL LLVM

Event Timeline

tromey created this revision.Nov 1 2017, 1:05 PM
JDevlieghere set the repository for this revision to rL LLVM.
JDevlieghere added inline comments.Nov 1 2017, 1:58 PM
test/DebugInfo/Generic/containing-type-extension.ll
8 ↗(On Diff #121168)

You can omit the regex at the end of the line, FileCheck will look for the substring, similar to grep.

9 ↗(On Diff #121168)

You should use CHECK-NEXT: or add something like CHECK-NOT: DW_TAG between your CHECK lines to ensure that they actually belong to this DW_TAG_structure_type.

aprantl added inline comments.Nov 1 2017, 2:02 PM
test/DebugInfo/Generic/containing-type-extension.ll
7 ↗(On Diff #121168)

you probably want a CHECK-NOT: TAG here to avoid matching beyond the current tag
alternatively you can use llvm-dwarfdump --name vtable

19 ↗(On Diff #121168)

we usually strip out all non-essential attributes (everything in quotes) from testcases.

24 ↗(On Diff #121168)

Why not use DW_LANG_Rust and an actual example?

unittests/IR/MetadataTest.cpp
1317 ↗(On Diff #121168)

Maybe you can specifically call out Rust as a user here.

tromey added inline comments.Nov 2 2017, 7:45 AM
test/DebugInfo/Generic/containing-type-extension.ll
24 ↗(On Diff #121168)

It seemed better to me to have a minimal example; but I can make the DW_LANG_Rust change pretty easily.

tromey marked 7 inline comments as done.Nov 2 2017, 7:49 AM
tromey updated this revision to Diff 121328.Nov 2 2017, 9:20 AM

Address review comments

aprantl added inline comments.Nov 2 2017, 9:31 AM
test/DebugInfo/Generic/containing-type-extension.ll
24 ↗(On Diff #121168)

My point was: it would be helpful to have actual Rust debug info code in the testcase to serve as a reminder hat kind of use-cases we need to support to guide future development/refactoring of LLVM. Having a minimal testcase is of course good, too, but I think in this case it would be nice to have an example of actual well-formed Rust debug info highlighting what Rust needs (and how it is different from clang). Can you generate a similarly small testcase from Rust?

tromey added inline comments.Nov 2 2017, 9:57 AM
test/DebugInfo/Generic/containing-type-extension.ll
24 ↗(On Diff #121168)

Sure, I can. The one in the patch was made by minimizing a rustc-generated file. In one real .ll I have:

!3 = !DICompositeType(tag: DW_TAG_structure_type, name: "vtable", file: !2, size: 64, align: 64, flags: DIFlagArtificial, elements: !4, vtableHolder: !5, identifier: "vtable")
...
!5 = !DIBasicType(name: "f64", size: 64, encoding: DW_ATE_float)

... which I think is essentially the same as what's in the current test:

!8 = !DIBasicType(tag: DW_TAG_base_type, name: "int", size: 32, align: 32, encoding: DW_ATE_signed)
...
!12 = !DICompositeType(tag: DW_TAG_structure_type, name: "vtable", size: 8, align: 8, elements: !2, identifier: "vtable", vtableHolder: !8)
tromey added inline comments.Nov 2 2017, 11:19 AM
test/DebugInfo/Generic/containing-type-extension.ll
6 ↗(On Diff #121328)

This comment is wrong - copied from a different test. I'll fix it.

tromey updated this revision to Diff 121340.Nov 2 2017, 11:23 AM

Add a test based on rustc output

tromey marked an inline comment as done.Nov 2 2017, 11:23 AM
tromey marked 2 inline comments as done.
JDevlieghere accepted this revision.EditedNov 8 2017, 2:19 AM

Looks good to me with these few nits, but let's wait for Adrian's approval before landing.

test/DebugInfo/Generic/containing-type-extension-rust.ll
9 ↗(On Diff #121340)

You should also add CHECK-NOT: TAG below this line if the DW_AT_name should belong to the same DW_TAG_structure_type

test/DebugInfo/Generic/containing-type-extension.ll
9 ↗(On Diff #121340)

Same here.

This revision is now accepted and ready to land.Nov 8 2017, 2:19 AM
aprantl accepted this revision.Nov 8 2017, 9:54 AM
tromey added a comment.Nov 8 2017, 1:32 PM

Thanks for the reviews. I don't have commit access, so could someone please check this in for me?

This revision was automatically updated to reflect the committed changes.