From 305bf1245e112e16a146b119ea11eec059cc75d9 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 21 Oct 2019 15:45:09 -0700 Subject: [PATCH 1/6] Test for 1-based columns in debuginfo --- src/test/codegen/debug-column-1-based.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 src/test/codegen/debug-column-1-based.rs diff --git a/src/test/codegen/debug-column-1-based.rs b/src/test/codegen/debug-column-1-based.rs new file mode 100644 index 0000000000000..2599da073f992 --- /dev/null +++ b/src/test/codegen/debug-column-1-based.rs @@ -0,0 +1,22 @@ +// Ensure that columns emitted in DWARF are 1-based + +// ignore-windows +// compile-flags: -g -C no-prepopulate-passes + +#![allow(dead_code)] +#![allow(unused_variables)] +#![allow(unused_assignments)] + +fn main() { // 10 + let x = 32; // 11 +} // 12 +// | | +// | | +// 5 9 column index + +// CHECK-LABEL: !DISubprogram(name: "main" +// CHECK: !DILocalVariable(name: "x",{{.*}}line: 11{{[^0-9]}} +// CHECK-NEXT: !DILexicalBlock({{.*}}line: 11, column: 5{{[^0-9]}} +// CHECK-NEXT: !DILocation(line: 11, column: 9{{[^0-9]}} +// CHECK: !DILocation(line: 12, column: 1{{[^0-9]}} +// CHECK-EMPTY: From 1d50972101c4cec1de47576d8f8833f08618d20d Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 21 Oct 2019 15:45:27 -0700 Subject: [PATCH 2/6] Emit 1-based column indexes for locations in debuginfo --- src/librustc_codegen_llvm/debuginfo/source_loc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_codegen_llvm/debuginfo/source_loc.rs b/src/librustc_codegen_llvm/debuginfo/source_loc.rs index ccb3bde1cbe4e..a751b615508d8 100644 --- a/src/librustc_codegen_llvm/debuginfo/source_loc.rs +++ b/src/librustc_codegen_llvm/debuginfo/source_loc.rs @@ -60,7 +60,7 @@ pub fn set_debug_location( let col_used = if bx.sess().target.target.options.is_like_msvc { UNKNOWN_COLUMN_NUMBER } else { - col as c_uint + (col + 1) as c_uint }; debug!("setting debug location to {} {}", line, col); From 24fbb30da37b2e4d68552238518eaeb67f3d82c2 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Mon, 21 Oct 2019 17:23:11 -0700 Subject: [PATCH 3/6] Emit 1-based column indexes for source scopes in debuginfo --- src/librustc_codegen_llvm/debuginfo/create_scope_map.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/librustc_codegen_llvm/debuginfo/create_scope_map.rs b/src/librustc_codegen_llvm/debuginfo/create_scope_map.rs index 6ee76b71fced6..f9c21d2fa0da2 100644 --- a/src/librustc_codegen_llvm/debuginfo/create_scope_map.rs +++ b/src/librustc_codegen_llvm/debuginfo/create_scope_map.rs @@ -81,13 +81,14 @@ fn make_mir_scope(cx: &CodegenCx<'ll, '_>, &loc.file.name, debug_context.defining_crate); + let col1 = loc.col.to_usize() + 1; // One-based column index let scope_metadata = unsafe { Some(llvm::LLVMRustDIBuilderCreateLexicalBlock( DIB(cx), parent_scope.scope_metadata.unwrap(), file_metadata, loc.line as c_uint, - loc.col.to_usize() as c_uint)) + col1 as c_uint)) }; debug_context.scopes[scope] = DebugScope { scope_metadata, From 49fc4205d10cea150c28e51d9945623596a7edc4 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 22 Oct 2019 09:30:12 -0700 Subject: [PATCH 4/6] Add `is_empty` method to `Span` --- src/libsyntax_pos/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libsyntax_pos/lib.rs b/src/libsyntax_pos/lib.rs index 9034f8c1afd1b..968ea2acd60a4 100644 --- a/src/libsyntax_pos/lib.rs +++ b/src/libsyntax_pos/lib.rs @@ -280,6 +280,13 @@ impl Span { self.data().with_ctxt(ctxt) } + #[inline] + /// Returns `true` if this `Span` contains no text. + pub fn is_empty(self) -> bool { + let span = self.data(); + span.lo == span.hi + } + /// Returns `true` if this is a dummy span with any hygienic context. #[inline] pub fn is_dummy(self) -> bool { From 4f38f11f7a2b39556ffa2c1d2376f33d85fed7ae Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 22 Oct 2019 09:30:42 -0700 Subject: [PATCH 5/6] Use preceding column for zero-width spans --- src/librustc_codegen_llvm/debuginfo/mod.rs | 8 ++-- .../debuginfo/source_loc.rs | 41 +++++++++++++++---- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/librustc_codegen_llvm/debuginfo/mod.rs b/src/librustc_codegen_llvm/debuginfo/mod.rs index 7713fe47004b9..4a6021e4a38da 100644 --- a/src/librustc_codegen_llvm/debuginfo/mod.rs +++ b/src/librustc_codegen_llvm/debuginfo/mod.rs @@ -35,7 +35,7 @@ use std::cell::RefCell; use std::ffi::{CStr, CString}; use smallvec::SmallVec; -use syntax_pos::{self, BytePos, Span, Pos}; +use syntax_pos::{self, BytePos, Span}; use syntax::ast; use syntax::symbol::Symbol; use rustc::ty::layout::{self, LayoutOf, HasTyCtxt, Size}; @@ -209,8 +209,10 @@ impl DebugInfoBuilderMethods<'tcx> for Builder<'a, 'll, 'tcx> { align.bytes() as u32, ) }; - source_loc::set_debug_location(self, - InternalDebugLocation::new(scope_metadata, loc.line, loc.col.to_usize())); + + let debug_loc = InternalDebugLocation::from_span(cx, scope_metadata, span); + source_loc::set_debug_location(self, debug_loc); + unsafe { let debug_loc = llvm::LLVMGetCurrentDebugLocation(self.llbuilder); let instr = llvm::LLVMRustDIBuilderInsertDeclareAtEnd( diff --git a/src/librustc_codegen_llvm/debuginfo/source_loc.rs b/src/librustc_codegen_llvm/debuginfo/source_loc.rs index a751b615508d8..e755b027a7881 100644 --- a/src/librustc_codegen_llvm/debuginfo/source_loc.rs +++ b/src/librustc_codegen_llvm/debuginfo/source_loc.rs @@ -1,14 +1,17 @@ use self::InternalDebugLocation::*; -use super::utils::{debug_context, span_start}; +use super::utils::debug_context; use super::metadata::UNKNOWN_COLUMN_NUMBER; use rustc_codegen_ssa::mir::debuginfo::FunctionDebugContext; use crate::llvm; use crate::llvm::debuginfo::DIScope; use crate::builder::Builder; +use crate::common::CodegenCx; use rustc_codegen_ssa::traits::*; +use std::num::NonZeroUsize; + use libc::c_uint; use syntax_pos::{Span, Pos}; @@ -23,8 +26,7 @@ pub fn set_source_location( ) { let dbg_loc = if debug_context.source_locations_enabled { debug!("set_source_location: {}", bx.sess().source_map().span_to_string(span)); - let loc = span_start(bx.cx(), span); - InternalDebugLocation::new(scope, loc.line, loc.col.to_usize()) + InternalDebugLocation::from_span(bx.cx(), scope, span) } else { UnknownLocation }; @@ -34,18 +36,43 @@ pub fn set_source_location( #[derive(Copy, Clone, PartialEq)] pub enum InternalDebugLocation<'ll> { - KnownLocation { scope: &'ll DIScope, line: usize, col: usize }, + KnownLocation { + scope: &'ll DIScope, + line: usize, + col: Option, + }, UnknownLocation } impl InternalDebugLocation<'ll> { - pub fn new(scope: &'ll DIScope, line: usize, col: usize) -> Self { + pub fn new(scope: &'ll DIScope, line: usize, col: Option) -> Self { KnownLocation { scope, line, col, } } + + pub fn from_span(cx: &CodegenCx<'ll, '_>, scope: &'ll DIScope, span: Span) -> Self { + let pos = cx.sess().source_map().lookup_char_pos(span.lo()); + + // FIXME: Rust likes to emit zero-width spans just after the end of a function. For now, + // zero-width spans to the preceding column to avoid emitting a column that points past the + // end of a line. E.g., + // |xyz = None + // x|yz = 1 + // xy|z = 2 + // + // See discussion in https://github.com/rust-lang/rust/issues/65437 + let col0 = pos.col.to_usize(); + let col1 = if span.is_empty() { + NonZeroUsize::new(col0) + } else { + NonZeroUsize::new(col0 + 1) + }; + + Self::new(scope, pos.line, col1) + } } pub fn set_debug_location( @@ -60,9 +87,9 @@ pub fn set_debug_location( let col_used = if bx.sess().target.target.options.is_like_msvc { UNKNOWN_COLUMN_NUMBER } else { - (col + 1) as c_uint + col.map_or(UNKNOWN_COLUMN_NUMBER, |c| c.get() as c_uint) }; - debug!("setting debug location to {} {}", line, col); + debug!("setting debug location to {} {}", line, col_used); unsafe { Some(llvm::LLVMRustDIBuilderCreateDebugLocation( From f24bb997f99e7904fd4be318f39545f57abbe12e Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 30 Oct 2019 08:23:30 -0700 Subject: [PATCH 6/6] Clarify comment in about columns for zero-width spans --- .../debuginfo/source_loc.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/librustc_codegen_llvm/debuginfo/source_loc.rs b/src/librustc_codegen_llvm/debuginfo/source_loc.rs index e755b027a7881..c34dc3e977176 100644 --- a/src/librustc_codegen_llvm/debuginfo/source_loc.rs +++ b/src/librustc_codegen_llvm/debuginfo/source_loc.rs @@ -56,14 +56,19 @@ impl InternalDebugLocation<'ll> { pub fn from_span(cx: &CodegenCx<'ll, '_>, scope: &'ll DIScope, span: Span) -> Self { let pos = cx.sess().source_map().lookup_char_pos(span.lo()); - // FIXME: Rust likes to emit zero-width spans just after the end of a function. For now, - // zero-width spans to the preceding column to avoid emitting a column that points past the - // end of a line. E.g., - // |xyz = None - // x|yz = 1 - // xy|z = 2 + // Rust likes to emit zero-width spans that point just after a closing brace to denote e.g. + // implicit return from a function. Instead of mapping zero-width spans to the column at + // `span.lo` like we do normally, map them to the column immediately before the span. This + // ensures that we point to a closing brace in the common case, and that debuginfo doesn't + // point past the end of a line. A zero-width span at the very start of the line gets + // mapped to `0`, which is used to represent "no column information" in DWARF. For example, // - // See discussion in https://github.com/rust-lang/rust/issues/65437 + // Span len = 0 Span len = 1 + // + // |xyz => 0 (None) |x|yz => 1 + // x|yz => 1 x|y|z => 2 + // + // See discussion in https://github.com/rust-lang/rust/issues/65437 for more info. let col0 = pos.col.to_usize(); let col1 = if span.is_empty() { NonZeroUsize::new(col0)