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

debuginfo: Build scope map for consts #22936

Closed
wants to merge 1 commit into from

Conversation

klutzy
Copy link
Contributor

@klutzy klutzy commented Mar 1, 2015

Fixes #22432.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@klutzy
Copy link
Contributor Author

klutzy commented Mar 1, 2015

cc @michaelwoerister

@klutzy
Copy link
Contributor Author

klutzy commented Mar 1, 2015

This is fixup of #21744: it optimized rustc_trans::trans::expr::trans_into s.t. if expr is ExprPath referring const, get const expr and directly call trans_into. But debuginfo was not updated so it ignored const-ExprPath while building scope_map.

@eddyb
Copy link
Member

eddyb commented Mar 1, 2015

My bad - but this needs a test, otherwise it could regress again.

@michaelwoerister
Copy link
Member

Thanks for the PR. Unfortunately this solution will lead to wrong debug locations if the same constant is referred to more than once within the same function. The scope map uses node IDs as keys and for a constant expression these are always the same, regardless of where it gets expanded to. So later occurences will overwrite scope map entries of former ones.

I already have an idea how to solve this problem differently.

@michaelwoerister
Copy link
Member

OK, so my idea is to provide debuginfo::with_debug_loc_override() function that allows to fixate emitted debug locations to a given value for a limited scope. This can then be used in expr::trans_into(), where it allows to set all emitted debug locations to the location of the constant's occurrence. To me this seems like a reasonably clean solution.

@klutzy Let me know if you have a better idea or want to implement my suggestion. Otherwise I'll implement it.

@michaelwoerister
Copy link
Member

@klutzy Since I haven't heard from you, I implemented my suggested solution (PR #23074).

@klutzy
Copy link
Contributor Author

klutzy commented Mar 5, 2015

@michaelwoerister I haven't have sufficient time to investigate this further, sorry. Thanks for making #23074!

@klutzy klutzy closed this Mar 5, 2015
@klutzy klutzy deleted the issue-22432 branch March 7, 2015 06:56
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.

Internal compiler error at compiling openssl-sys-0.4.0
4 participants