Auto merge of #32952 - eddyb:mir-debuginfo-2, r=michaelwoerister

Get all (but one) of debuginfo tests to pass with MIR codegen.

I didn't get much feedback in #31005 so I went ahead and implemented something simple.
Closes #31005, as MIR debuginfo should work now for most usecases.

The `no-debug-attribute` test no longer assumes variables are in scope of `return`.
We might also want to revisit that in #32949, but the test is more reliable now either way.

In order to get one last function in the `associated-type` test pass, this PR also fixes #32790.

src/librustc/mir/repr.rs

@@ -52,6 +52,10 @@ pub struct Mir<'tcx> {
     /// through the resulting reference.
     pub temp_decls: Vec<TempDecl<'tcx>>,
 
+    /// 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>,
+
     /// A span representing this MIR, for error reporting
     pub span: Span,
 }
@@ -197,7 +201,20 @@ pub struct ArgDecl<'tcx> {
 
     /// If true, this argument is a tuple after monomorphization,
     /// and has to be collected from multiple actual arguments.
-    pub spread: bool
+    pub spread: bool,
+
+    /// Either special_idents::invalid or the name of a single-binding
+    /// pattern associated with this argument. Useful for debuginfo.
+    pub debug_name: Name
+}
+
+/// A closure capture, with its name and mode.
+#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
+pub struct UpvarDecl {
+    pub debug_name: Name,
+
+    /// If true, the capture is behind a reference.
+    pub by_ref: bool
 }
 
 ///////////////////////////////////////////////////////////////////////////

src/librustc/mir/visit.rs

@@ -248,6 +248,7 @@ macro_rules! make_mir_visitor {
                     ref $($mutability)* var_decls,
                     ref $($mutability)* arg_decls,
                     ref $($mutability)* temp_decls,
+                    upvar_decls: _,
                     ref $($mutability)* span,
                 } = *mir;
 
@@ -599,7 +600,8 @@ macro_rules! make_mir_visitor {
                               arg_decl: & $($mutability)* ArgDecl<'tcx>) {
                 let ArgDecl {
                     ref $($mutability)* ty,
-                    spread: _
+                    spread: _,
+                    debug_name: _
                 } = *arg_decl;
 
                 self.visit_ty(ty);

src/librustc_mir/build/matches/mod.rs

@@ -37,25 +37,28 @@ impl<'a,'tcx> Builder<'a,'tcx> {
                       -> BlockAnd<()> {
         let discriminant_lvalue = unpack!(block = self.as_lvalue(block, discriminant));
 
-        // Before we do anything, create uninitialized variables with
-        // suitable extent for all of the bindings in this match. It's
-        // easiest to do this up front because some of these arms may
-        // be unreachable or reachable multiple times.
-        let var_scope_id = self.innermost_scope_id();
-        for arm in &arms {
-            self.declare_bindings(var_scope_id, &arm.patterns[0]);
-        }
-
         let mut arm_blocks = ArmBlocks {
             blocks: arms.iter()
                         .map(|_| self.cfg.start_new_block())
                         .collect(),
         };
 
-        let arm_bodies: Vec<ExprRef<'tcx>> =
-            arms.iter()
-                .map(|arm| arm.body.clone())
-                .collect();
+        // Get the body expressions and their scopes, while declaring bindings.
+        let arm_bodies: Vec<_> = arms.iter().enumerate().map(|(i, arm)| {
+            // Assume that all expressions are wrapped in Scope.
+            let body = self.hir.mirror(arm.body.clone());
+            match body.kind {
+                ExprKind::Scope { extent, value } => {
+                    let scope_id = self.push_scope(extent, arm_blocks.blocks[i]);
+                    self.declare_bindings(scope_id, &arm.patterns[0]);
+                    (extent, self.scopes.pop().unwrap(), value)
+                }
+                _ => {
+                    span_bug!(body.span, "arm body is not wrapped in Scope {:?}",
+                              body.kind);
+                }
+            }
+        }).collect();
 
         // assemble a list of candidates: there is one candidate per
         // pattern, which means there may be more than one candidate
@@ -95,11 +98,15 @@ impl<'a,'tcx> Builder<'a,'tcx> {
         // all the arm blocks will rejoin here
         let end_block = self.cfg.start_new_block();
 
-        for (arm_index, arm_body) in arm_bodies.into_iter().enumerate() {
+        let scope_id = self.innermost_scope_id();
+        for (arm_index, (extent, scope, body)) in arm_bodies.into_iter().enumerate() {
             let mut arm_block = arm_blocks.blocks[arm_index];
-            unpack!(arm_block = self.into(destination, arm_block, arm_body));
+            // Re-enter the scope we created the bindings in.
+            self.scopes.push(scope);
+            unpack!(arm_block = self.into(destination, arm_block, body));
+            unpack!(arm_block = self.pop_scope(extent, arm_block));
             self.cfg.terminate(arm_block,
-                               var_scope_id,
+                               scope_id,
                                span,
                                TerminatorKind::Goto { target: end_block });
         }

src/librustc_mir/build/mod.rs

@@ -10,13 +10,15 @@
 
 use hair::cx::Cx;
 use rustc::middle::region::{CodeExtent, CodeExtentData};
-use rustc::ty::{FnOutput, Ty};
+use rustc::ty::{self, FnOutput, Ty};
 use rustc::mir::repr::*;
 use rustc_data_structures::fnv::FnvHashMap;
 use rustc::hir;
+use rustc::hir::pat_util::pat_is_binding;
 use std::ops::{Index, IndexMut};
 use syntax::ast;
 use syntax::codemap::Span;
+use syntax::parse::token;
 
 pub struct Builder<'a, 'tcx: 'a> {
     hir: Cx<'a, 'tcx>,
@@ -224,13 +226,37 @@ pub fn construct<'a,'tcx>(hir: Cx<'a,'tcx>,
                        true
                    }));
 
+    // Gather the upvars of a closure, if any.
+    let upvar_decls: Vec<_> = tcx.with_freevars(fn_id, |freevars| {
+        freevars.iter().map(|fv| {
+            let by_ref = tcx.upvar_capture(ty::UpvarId {
+                var_id: fv.def.var_id(),
+                closure_expr_id: fn_id
+            }).map_or(false, |capture| match capture {
+                ty::UpvarCapture::ByValue => false,
+                ty::UpvarCapture::ByRef(..) => true
+            });
+            let mut decl = UpvarDecl {
+                debug_name: token::special_idents::invalid.name,
+                by_ref: by_ref
+            };
+            if let Some(hir::map::NodeLocal(pat)) = tcx.map.find(fv.def.var_id()) {
+                if let hir::PatKind::Ident(_, ref ident, _) = pat.node {
+                    decl.debug_name = ident.node.name;
+                }
+            }
+            decl
+        }).collect()
+    });
+
     (
         Mir {
             basic_blocks: builder.cfg.basic_blocks,
             scopes: builder.scope_datas,
             var_decls: builder.var_decls,
             arg_decls: arg_decls.take().expect("args never built?"),
             temp_decls: builder.temp_decls,
+            upvar_decls: upvar_decls,
             return_ty: return_ty,
             span: span
         },
@@ -269,7 +295,20 @@ impl<'a,'tcx> Builder<'a,'tcx> {
                 self.schedule_drop(pattern.as_ref().map_or(ast_block.span, |pat| pat.span),
                                    argument_extent, &lvalue, ty);
 
-                ArgDecl { ty: ty, spread: false }
+                let mut name = token::special_idents::invalid.name;
+                if let Some(pat) = pattern {
+                    if let hir::PatKind::Ident(_, ref ident, _) = pat.node {
+                        if pat_is_binding(&self.hir.tcx().def_map.borrow(), pat) {
+                            name = ident.node.name;
+                        }
+                    }
+                }
+
+                ArgDecl {
+                    ty: ty,
+                    spread: false,
+                    debug_name: name
+                }
             })
             .collect();
 

src/librustc_trans/debuginfo/create_scope_map.rs

@@ -120,21 +120,28 @@ fn make_mir_scope(ccx: &CrateContext,
         return;
     };
 
-    scopes[idx] = if !has_variables.contains(idx) {
+    if !has_variables.contains(idx) {
         // Do not create a DIScope if there are no variables
         // defined in this MIR Scope, to avoid debuginfo bloat.
-        parent_scope
-    } else {
-        let loc = span_start(ccx, scope_data.span);
-        let file_metadata = file_metadata(ccx, &loc.file.name);
-        unsafe {
-            llvm::LLVMDIBuilderCreateLexicalBlock(
-                DIB(ccx),
-                parent_scope,
-                file_metadata,
-                loc.line as c_uint,
-                loc.col.to_usize() as c_uint)
+
+        // However, we don't skip creating a nested scope if
+        // our parent is the root, because we might want to
+        // put arguments in the root and not have shadowing.
+        if parent_scope != fn_metadata {
+            scopes[idx] = parent_scope;
+            return;
         }
+    }
+
+    let loc = span_start(ccx, scope_data.span);
+    let file_metadata = file_metadata(ccx, &loc.file.name);
+    scopes[idx] = unsafe {
+        llvm::LLVMDIBuilderCreateLexicalBlock(
+            DIB(ccx),
+            parent_scope,
+            file_metadata,
+            loc.line as c_uint,
+            loc.col.to_usize() as c_uint)
     };
 }
 

src/librustc_trans/mir/mod.rs

@@ -126,6 +126,7 @@ pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) {
     let scopes = debuginfo::create_mir_scopes(fcx);
 
     // Allocate variable and temp allocas
+    let args = arg_value_refs(&bcx, &mir, &scopes);
     let vars = mir.var_decls.iter()
                             .map(|decl| (bcx.monomorphize(&decl.ty), decl))
                             .map(|(mty, decl)| {
@@ -156,7 +157,6 @@ pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) {
                                   TempRef::Operand(None)
                               })
                               .collect();
-    let args = arg_value_refs(&bcx, &mir, &scopes);
 
     // Allocate a `Block` for every basic block
     let block_bcxs: Vec<Block<'blk,'tcx>> =
@@ -278,15 +278,15 @@ fn arg_value_refs<'bcx, 'tcx>(bcx: &BlockAndBuilder<'bcx, 'tcx>,
                     let byte_offset_of_var_in_tuple =
                         machine::llelement_offset(bcx.ccx(), lltuplety, i);
 
-                    let address_operations = unsafe {
+                    let ops = unsafe {
                         [llvm::LLVMDIBuilderCreateOpDeref(),
                          llvm::LLVMDIBuilderCreateOpPlus(),
                          byte_offset_of_var_in_tuple as i64]
                     };
 
                     let variable_access = VariableAccess::IndirectVariable {
                         alloca: lltemp,
-                        address_operations: &address_operations
+                        address_operations: &ops
                     };
                     declare_local(bcx, token::special_idents::invalid.name,
                                   tupled_arg_ty, scope, variable_access,
@@ -327,10 +327,78 @@ fn arg_value_refs<'bcx, 'tcx>(bcx: &BlockAndBuilder<'bcx, 'tcx>,
             lltemp
         };
         bcx.with_block(|bcx| arg_scope.map(|scope| {
-            declare_local(bcx, token::special_idents::invalid.name, arg_ty, scope,
-                          VariableAccess::DirectVariable { alloca: llval },
-                          VariableKind::ArgumentVariable(arg_index + 1),
-                          bcx.fcx().span.unwrap_or(DUMMY_SP));
+            // Is this a regular argument?
+            if arg_index > 0 || mir.upvar_decls.is_empty() {
+                declare_local(bcx, arg_decl.debug_name, arg_ty, scope,
+                              VariableAccess::DirectVariable { alloca: llval },
+                              VariableKind::ArgumentVariable(arg_index + 1),
+                              bcx.fcx().span.unwrap_or(DUMMY_SP));
+                return;
+            }
+
+            // Or is it the closure environment?
+            let (closure_ty, env_ref) = if let ty::TyRef(_, mt) = arg_ty.sty {
+                (mt.ty, true)
+            } else {
+                (arg_ty, false)
+            };
+            let upvar_tys = if let ty::TyClosure(_, ref substs) = closure_ty.sty {
+                &substs.upvar_tys[..]
+            } else {
+                bug!("upvar_decls with non-closure arg0 type `{}`", closure_ty);
+            };
+
+            // Store the pointer to closure data in an alloca for debuginfo
+            // because that's what the llvm.dbg.declare intrinsic expects.
+
+            // FIXME(eddyb) this shouldn't be necessary but SROA seems to
+            // mishandle DW_OP_plus not preceded by DW_OP_deref, i.e. it
+            // doesn't actually strip the offset when splitting the closure
+            // environment into its components so it ends up out of bounds.
+            let env_ptr = if !env_ref {
+                use base::*;
+                use build::*;
+                use common::*;
+                let alloc = alloca(bcx, val_ty(llval), "__debuginfo_env_ptr");
+                Store(bcx, llval, alloc);
+                alloc
+            } else {
+                llval
+            };
+
+            let llclosurety = type_of::type_of(bcx.ccx(), closure_ty);
+            for (i, (decl, ty)) in mir.upvar_decls.iter().zip(upvar_tys).enumerate() {
+                let byte_offset_of_var_in_env =
+                    machine::llelement_offset(bcx.ccx(), llclosurety, i);
+
+                let ops = unsafe {
+                    [llvm::LLVMDIBuilderCreateOpDeref(),
+                     llvm::LLVMDIBuilderCreateOpPlus(),
+                     byte_offset_of_var_in_env as i64,
+                     llvm::LLVMDIBuilderCreateOpDeref()]
+                };
+
+                // The environment and the capture can each be indirect.
+
+                // FIXME(eddyb) see above why we have to keep
+                // a pointer in an alloca for debuginfo atm.
+                let mut ops = if env_ref || true { &ops[..] } else { &ops[1..] };
+
+                let ty = if let (true, &ty::TyRef(_, mt)) = (decl.by_ref, &ty.sty) {
+                    mt.ty
+                } else {
+                    ops = &ops[..ops.len() - 1];
+                    ty
+                };
+
+                let variable_access = VariableAccess::IndirectVariable {
+                    alloca: env_ptr,
+                    address_operations: &ops
+                };
+                declare_local(bcx, decl.debug_name, ty, scope, variable_access,
+                              VariableKind::CapturedVariable,
+                              bcx.fcx().span.unwrap_or(DUMMY_SP));
+            }
         }));
         LvalueRef::new_sized(llval, LvalueTy::from_ty(arg_ty))
     }).collect()

src/test/debuginfo/associated-types.rs

@@ -80,7 +80,7 @@
 
 #![allow(unused_variables)]
 #![allow(dead_code)]
-#![feature(omit_gdb_pretty_printer_section, rustc_attrs)]
+#![feature(omit_gdb_pretty_printer_section)]
 #![omit_gdb_pretty_printer_section]
 
 trait TraitWithAssocType {
@@ -127,7 +127,6 @@ fn assoc_tuple<T: TraitWithAssocType>(arg: (T, T::Type)) {
     zzz(); // #break
 }
 
-#[rustc_no_mir] // FIXME(#32790) MIR reuses scopes for match arms.
 fn assoc_enum<T: TraitWithAssocType>(arg: Enum<T>) {
 
     match arg {