Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Trait object debug #45897

Merged
merged 2 commits into from
Nov 17, 2017
Merged

Trait object debug #45897

merged 2 commits into from
Nov 17, 2017

Conversation

tromey
Copy link
Contributor

@tromey tromey commented Nov 9, 2017

This enables better debugging of trait objects. See the individual commits for explanations. This required an LLVM bump.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 10, 2017
@bors
Copy link
Contributor

bors commented Nov 11, 2017

☔ The latest upstream changes (presumably #45806) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

r? @michaelwoerister

michaelwoerister
michaelwoerister approved these changes Nov 13, 2017
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks @tromey!

@tromey tromey force-pushed the trait-object-debug branch 2 times, most recently from 8c9a797 to ab4da8c Compare November 13, 2017 19:11
@tromey
Copy link
Contributor Author

tromey commented Nov 14, 2017

So far I haven't managed to reproduce that failure.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2017
@tromey
Copy link
Contributor Author

tromey commented Nov 14, 2017

My current theory is that this is failing due to an unmodified LLVM; and so the patch will need some tweaks.

@tromey
Copy link
Contributor Author

tromey commented Nov 14, 2017

Looking at the patches, I still don't see how that would be possible, but I'm rebuilding without the LLVM patch to be sure.

@tromey
Copy link
Contributor Author

tromey commented Nov 14, 2017

I rebuilt without the LLVM bump and didn't see this failure. Now I don't have a theory about what is going wrong. Maybe I need to build specifically against whatever LLVM travis is using.

@cuviper
Copy link
Member

cuviper commented Nov 14, 2017

The first CI run uses Ubuntu's llvm-3.9. The rest should use rust's own llvm.

@tromey
Copy link
Contributor Author

tromey commented Nov 15, 2017

The first CI run uses Ubuntu's llvm-3.9

Thanks. I got it to fail with my system llvm.

@tromey
Copy link
Contributor Author

tromey commented Nov 15, 2017

The bug is that the patch is passing in NULL for the linkage name of the vtable.

@tromey
Allow minimum system LLVM version in tests
a512228 
This adds a "min-system-llvm-version" directive, so that a test can
indicate that it will either work with rust-llvm or with some minimal
system LLVM.  This makes it simpler to write a test that requires an
LLVM patch that landed upstream and was then backported to rust-llvm.
@tromey
Emit debug info for trait object pointer
ae4cc60 
Emit better debugging information for a trait object pointer.  In
particular, now:

* The fields are explicitly represented in the DWARF;
* DWARF for the vtable itself is emitted; and
* The DWARF for the vtable's type has a DW_AT_containing_type which
  points to the concrete type for which the vtable was emitted.  This is
  a small DWARF extension, that allows debuggers to determine the real
  type of the object to which a trait object points.

I'll submit the gdb patch to take advantage of this new debuginfo once
this lands.

The vtable type is not currently complete -- it doesn't include members
for the pointers it contains.  This information was not needed for this
feature.

This addresses part 1 of rust-lang#1563.
@tromey tromey force-pushed the trait-object-debug branch from ab4da8c to ae4cc60 Compare November 15, 2017 08:48
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 15, 2017
@kennytm
Copy link
Member

kennytm commented Nov 15, 2017

@tromey I think this is ready to be reviewed or merged? cc @michaelwoerister

@tromey
Copy link
Contributor Author

tromey commented Nov 15, 2017

@kennytm yes - it was reviewed once, then I made a small change to address the test failures. A re-review would be good.

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2017

📌 Commit ae4cc60 has been approved by michaelwoerister

@kennytm kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 16, 2017
@bors
Copy link
Contributor

bors commented Nov 16, 2017

⌛ Testing commit ae4cc60 with merge d59f66d...

bors added a commit that referenced this pull request Nov 16, 2017
@bors
Auto merge of #45897 - tromey:trait-object-debug, r=michaelwoerister
d59f66d 
Trait object debug

This enables better debugging of trait objects.  See the individual commits for explanations.  This required an LLVM bump.
@bors
Copy link
Contributor

bors commented Nov 17, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing d59f66d to master...

@bors bors merged commit ae4cc60 into rust-lang:master Nov 17, 2017
@bors bors mentioned this pull request Nov 17, 2017
Merged
@tromey tromey deleted the trait-object-debug branch November 17, 2017 05:34
@cuviper cuviper mentioned this pull request Jun 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister approved these changes

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tromey @bors @alexcrichton @cuviper @kennytm @michaelwoerister