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

Future-proof MIR for dedicated debuginfo. #56278

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@eddyb
Member

eddyb commented Nov 27, 2018

This is #56231 without the last commit (the one that actually moves to VarDebuginfo).
Nothing should be broken, but it should no longer depend on debuginfo for anything else.

r? @nikomatsakis

@eddyb eddyb force-pushed the eddyb:mir-debuginfo-proof branch from b4c1d38 to 58ff04b Nov 29, 2018

@nikomatsakis

these changes look fine, though I'd be happier if we could make some nice queries for getting info about the Nth upvar of a closure, so that the code would be a bit cleaner

@@ -141,6 +142,40 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
.as_local_node_id(def_id)
.expect("do_mir_borrowck: non-local DefId");
// Gather the upvars of a closure, if any.
let upvars: Vec<_> = tcx.with_freevars(id, |freevars| {

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

This really feels like information that could be encoded in the MIR, no? Well, I guess I can see an argument both ways.

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

At minimum I would prefer if we can factor it out from borrow check into some kind of helper table that would be more re-usable.

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

But I don't know that I want to block the PR on it. In general I feel like the borrow check code is kind of noodly and could use some extractions of this kind.

/// Mark an argument local (which must be a tuple) as getting passed as
/// its individual components at the LLVM level.
///
/// This is used for the "rust-call" ABI.
pub spread_arg: Option<Local>,
/// Names and capture modes of all the closure upvars, assuming
/// the first argument is either the closure or a reference to it.
pub upvar_decls: Vec<UpvarDecl>,

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

btw, I think that if there are fields that are "debuginfo" that you feel other people should not rely on, we should comment that =)

(Like this one, judging from previous commit)

PathElem::ClosureVar(Symbol::intern(&field.to_string()))
let mut name = None;
if let Some(node_id) = self.ecx.tcx.hir.as_local_node_id(def_id) {
self.ecx.tcx.with_freevars(node_id, |freevars| {

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

I wonder if this has any overlap with the borrow check code I was noting earlier? (i.e., if some common helper could make both of these more concise)

// debuginfo generation, and will be removed at some point.
// Do **NOT** use it for anything else, upvar information should not be
// in the MIR, please rely on local crate HIR or other side-channels.
pub __upvar_debuginfo_codegen_only_do_not_use: Vec<UpvarDebuginfo>,

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

lol ok I guess that addresses my earlier concern

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

I would like this to be part of the doc comment please though, why use only //?

This comment has been minimized.

@eddyb

eddyb Nov 30, 2018

Member

I got used to only using // for NOTE/FIXME/HACK, but I'll go change it.

@@ -2208,3 +2277,15 @@ impl ContextKind {
}
}
}
#[derive(Debug)]
crate struct Upvar {

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 29, 2018

Contributor

Yeah I think I would prefer to see closure_upvars(def_id, n) -> Upvar that returns this information "nicely collected".... (or closure_upvars(def_id) -> &[Upvar]).

But I won't hold up the PR on it.

r=me, depending on how you feel @eddyb

@bors

This comment has been minimized.

Contributor

bors commented Dec 2, 2018

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

@Aaronepower

This comment has been minimized.

Contributor

Aaronepower commented Dec 10, 2018

Triage; @eddyb Hello, have you been able to get back to this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment