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.

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

Debug info #7134

Closed
wants to merge 11 commits into from
Closed

Debug info #7134

wants to merge 11 commits into from

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Jun 14, 2013

This commit fixes rustc's debug info generation and turns debug-info tests back on.

The old generator used to write out LLVM metadata directly, however it seems that debug metadata format is not stable and keeps changing from release to release. So I wrapped LLVM's official debug info API - the DIBuilder class, and now rustc will use that.

One bit of old functionality that still doesn't work, is debug info for function arguments. Someone more familiar with the compiler guts will need to look into that.

Also, unfortunately, debug info is still won't work on Windows,- due to a LLVM bug (http://llvm.org/bugs/show_bug.cgi?id=16249).

Resolves issues #5836, #5848, #6814

@Aatch
Copy link
Contributor

Aatch commented Jun 14, 2013

Awesome work, though is there are good reason you are using an @mut for the context? Ideally we want to try to get away from @-ptrs unnecessarily, using borrowed pointers instead. The usage in other parts of trans is mostly historical so I'm suspicious of new code using the same pattern (especially as I'm currently on a crusade to modernize trans).

struct Metadata<T> {
node: ValueRef,
data: T
#[inline(always)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need to be always inlined? I'd prefer you let LLVM decide here.

It probably doesn't matter here but over-use of #[inline(always)] is a bit of a problem in general that I'm trying to prevent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a fairly often used function, so it'd better be inlined. But if you vouch for LLVM optimizer, I have no problem removing that.

Copy link
Contributor

Choose a reason for hiding this comment

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

The optimizer does a pretty good job overall, with only a few cases (notably iterators) requiring a forced inline. Even just #[inline] would likely be enough.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jun 14, 2013

I need DebugContext to be mutable, and if I don't make it @mut, then the CrateContext would have to be passed by @mut, and that in turn would mean changing all code upstream of debuginfo to use @mut CrateContext as well, wouldn't it?

@Aatch
Copy link
Contributor

Aatch commented Jun 14, 2013

@vadimcn I meant more being more explicit, so passing by &mut or & when you can and be explicit with the @mut.

Though if you're willing to wait, I have a branch that does change CrateContext to be @mut. EDIT: I re-opened the PR: #7124

@jdm
Copy link
Contributor

jdm commented Jun 15, 2013

Just a request to hold off acting on this PR until I have a chance to look through it. I'm getting on a plane or I'd do so now.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jun 15, 2013

@Aatch: I can wait, though you guys have an intern starting on Monday, who's supposed to work on debug info. I wanted to land this before he gets in.

@Aatch
Copy link
Contributor

Aatch commented Jun 15, 2013

@vadimcn Well hopefully my PR will get in before then and @jdm will look at this before that as well.

@brson
Copy link
Contributor

brson commented Jun 16, 2013

This is a delightful surprise! Thanks @vadimcn.

@Aatch
Copy link
Contributor

Aatch commented Jun 16, 2013

@vadimcn the PR went through, so you can rebase now,

@jdm
Copy link
Contributor

jdm commented Jun 16, 2013

Sorry about the comments that have already been addressed in subsequent commits.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jun 16, 2013

@jdm: Thanks for the review, but it looks like you are reviewing only the first commit of my PR. Some comments still apply, but a lot of this stuff had been changed in later commits.
Anyways, I'm gonna rebase this PR to a point after Aatch's CrateContext change now, and will try to address outstanding issues.

@jdm
Copy link
Contributor

jdm commented Jun 16, 2013

Great work! I appreciate your efforts to get this done in time for Michael's first week on the project, too :) I'm happy for this to go in the tree after a rebase; my comments could be addressed in a subsequent PR.

@michaelwoerister
Copy link
Member

@vadimcn Thanks for all the work. Using the DIBuilder will hopefully shield us from most LLVM debug info format changes.
@vadimcn & @jdm How about I take over after the PR has gone through? I'd be happy to do the cleanup bits pointed out.

@vadimcn
Copy link
Contributor Author

vadimcn commented Jun 17, 2013

Ok, rebase is done.

bors added a commit that referenced this pull request Jun 17, 2013
This commit fixes rustc's debug info generation and turns debug-info tests back on.

The old generator used to write out LLVM metadata directly, however it seems that debug metadata format is not stable and keeps changing from release to release.  So I wrapped LLVM's official debug info API - the DIBuilder class, and now rustc will use that.

One bit of old functionality that still doesn't work, is debug info for function arguments.  Someone more familiar with the compiler guts will need to look into that.

Also, unfortunately, debug info is still won't work on Windows,- due to a LLVM bug (http://llvm.org/bugs/show_bug.cgi?id=16249).

Resolves issues #5836, #5848, #6814
@bors bors closed this Jun 17, 2013
@vadimcn
Copy link
Contributor Author

vadimcn commented Jun 18, 2013

@michaelwoerister: yes, please go ahead and take it over. Have fun!

@michaelwoerister
Copy link
Member

@vadimcn Thanks again! The code looks really good.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 27, 2021
Finish MSRV for cloned_instead_of_copied

changelog: none

r? `@Manishearth`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants