Auto merge of #42938 - est31:col_number, r=eddyb

Output column number info when panicking

Outputs the column number when panicking. Useful when you e.g. have code like `foo[i] = bar[k] + bar[l]` and you get a panic with index out of bounds, or when you have an expression like `a = b + c + d + e` and the addition overflows. Now you know which operation to blame!

The format is `file:line:column`, just like for compiler errors. Example output with the patch:

```
thread 'main' panicked at 'index out of bounds: the len is 5 but the index is 8', src/main.rs:3:8
```

As some of the API between the compiler and the library landscape gets broken, this is a bit hackier than I'd originally wanted it to be.

* `panic` and `panic_bounds_check` lang items got an additional column param, on stage0 I still have to use the previous version. After a SNAP this should be resolved.
* For `#[derive(RustcDeserialze)]`, stage0 requires a fixed signature for `std::rt::begin_panic`, so we can't change it right away. What we need to do instead is to keep the signature, and add a `begin_panic_new` function that we use in later stages instead. After a SNAP we can change the `begin_panic` function and rely on it instead of `begin_panic_new`, and one SNAP later we can remove `begin_panic_new`.
* Fortunately I didn't have to break anything about the panic hook API, I could easily extend it.

Note that debuginfo remains unchanged, so RUST_BACKTRACE output won't contain any column info. See issue #42921 for discussion on including the column in debuginfo.

Author: bors

src/Cargo.lock

@@ -143,7 +143,8 @@ pub extern fn rust_eh_unwind_resume() {
 #[no_mangle]
 pub extern fn rust_begin_panic(_msg: core::fmt::Arguments,
                                _file: &'static str,
-                               _line: u32) -> ! {
+                               _line: u32,
+                               _column: u32) -> ! {
     unsafe { intrinsics::abort() }
 }
 ```
@@ -187,7 +188,8 @@ pub extern fn rust_eh_unwind_resume() {
 #[no_mangle]
 pub extern fn rust_begin_panic(_msg: core::fmt::Arguments,
                                _file: &'static str,
-                               _line: u32) -> ! {
+                               _line: u32,
+                               _column: u32) -> ! {
     unsafe { intrinsics::abort() }
 }
 ```

src/doc/unstable-book/src/language-features/lang-items.md

@@ -36,12 +36,12 @@
 //!   These functions are often provided by the system libc, but can also be
 //!   provided by the [rlibc crate](https://crates.io/crates/rlibc).
 //!
-//! * `rust_begin_panic` - This function takes three arguments, a
-//!   `fmt::Arguments`, a `&'static str`, and a `u32`. These three arguments
+//! * `rust_begin_panic` - This function takes four arguments, a
+//!   `fmt::Arguments`, a `&'static str`, and two `u32`'s. These four arguments
 //!   dictate the panic message, the file at which panic was invoked, and the
-//!   line. It is up to consumers of this core library to define this panic
-//!   function; it is only required to never return. This requires a `lang`
-//!   attribute named `panic_fmt`.
+//!   line and column inside the file. It is up to consumers of this core
+//!   library to define this panic function; it is only required to never
+//!   return. This requires a `lang` attribute named `panic_fmt`.
 //!
 //! * `rust_eh_personality` - is used by the failure mechanisms of the
 //!    compiler. This is often mapped to GCC's personality function, but crates

src/libcore/lib.rs

@@ -17,16 +17,18 @@ macro_rules! panic {
         panic!("explicit panic")
     );
     ($msg:expr) => ({
-        static _MSG_FILE_LINE: (&'static str, &'static str, u32) = ($msg, file!(), line!());
-        $crate::panicking::panic(&_MSG_FILE_LINE)
+        static _MSG_FILE_LINE_COL: (&'static str, &'static str, u32, u32) =
+            ($msg, file!(), line!(), column!());
+        $crate::panicking::panic(&_MSG_FILE_LINE_COL)
     });
     ($fmt:expr, $($arg:tt)*) => ({
         // The leading _'s are to avoid dead code warnings if this is
         // used inside a dead function. Just `#[allow(dead_code)]` is
         // insufficient, since the user may have
         // `#[forbid(dead_code)]` and which cannot be overridden.
-        static _FILE_LINE: (&'static str, u32) = (file!(), line!());
-        $crate::panicking::panic_fmt(format_args!($fmt, $($arg)*), &_FILE_LINE)
+        static _MSG_FILE_LINE_COL: (&'static str, u32, u32) =
+            (file!(), line!(), column!());
+        $crate::panicking::panic_fmt(format_args!($fmt, $($arg)*), &_MSG_FILE_LINE_COL)
     });
 }
 

src/libcore/macros.rs

@@ -17,7 +17,7 @@
 //!
 //! ```
 //! # use std::fmt;
-//! fn panic_impl(fmt: fmt::Arguments, file_line: &(&'static str, u32)) -> !
+//! fn panic_impl(fmt: fmt::Arguments, file_line_col: &(&'static str, u32, u32)) -> !
 //! # { loop {} }
 //! ```
 //!
@@ -39,34 +39,55 @@
 use fmt;
 
 #[cold] #[inline(never)] // this is the slow path, always
-#[lang = "panic"]
-pub fn panic(expr_file_line: &(&'static str, &'static str, u32)) -> ! {
+#[cfg_attr(not(stage0), lang = "panic")]
+pub fn panic(expr_file_line_col: &(&'static str, &'static str, u32, u32)) -> ! {
     // Use Arguments::new_v1 instead of format_args!("{}", expr) to potentially
     // reduce size overhead. The format_args! macro uses str's Display trait to
     // write expr, which calls Formatter::pad, which must accommodate string
     // truncation and padding (even though none is used here). Using
     // Arguments::new_v1 may allow the compiler to omit Formatter::pad from the
     // output binary, saving up to a few kilobytes.
+    let (expr, file, line, col) = *expr_file_line_col;
+    panic_fmt(fmt::Arguments::new_v1(&[expr], &[]), &(file, line, col))
+}
+
+// FIXME: remove when SNAP
+#[cold] #[inline(never)]
+#[cfg(stage0)]
+#[lang = "panic"]
+pub fn panic_old(expr_file_line: &(&'static str, &'static str, u32)) -> ! {
     let (expr, file, line) = *expr_file_line;
-    panic_fmt(fmt::Arguments::new_v1(&[expr], &[]), &(file, line))
+    let expr_file_line_col = (expr, file, line, 0);
+    panic(&expr_file_line_col)
+}
+
+#[cold] #[inline(never)]
+#[cfg_attr(not(stage0), lang = "panic_bounds_check")]
+fn panic_bounds_check(file_line_col: &(&'static str, u32, u32),
+                     index: usize, len: usize) -> ! {
+    panic_fmt(format_args!("index out of bounds: the len is {} but the index is {}",
+                           len, index), file_line_col)
 }
 
+// FIXME: remove when SNAP
 #[cold] #[inline(never)]
+#[cfg(stage0)]
 #[lang = "panic_bounds_check"]
-fn panic_bounds_check(file_line: &(&'static str, u32),
+fn panic_bounds_check_old(file_line: &(&'static str, u32),
                      index: usize, len: usize) -> ! {
+    let (file, line) = *file_line;
     panic_fmt(format_args!("index out of bounds: the len is {} but the index is {}",
-                           len, index), file_line)
+                           len, index), &(file, line, 0))
 }
 
 #[cold] #[inline(never)]
-pub fn panic_fmt(fmt: fmt::Arguments, file_line: &(&'static str, u32)) -> ! {
+pub fn panic_fmt(fmt: fmt::Arguments, file_line_col: &(&'static str, u32, u32)) -> ! {
     #[allow(improper_ctypes)]
     extern {
         #[lang = "panic_fmt"]
         #[unwind]
-        fn panic_impl(fmt: fmt::Arguments, file: &'static str, line: u32) -> !;
+        fn panic_impl(fmt: fmt::Arguments, file: &'static str, line: u32, col: u32) -> !;
     }
-    let (file, line) = *file_line;
-    unsafe { panic_impl(fmt, file, line) }
+    let (file, line, col) = *file_line_col;
+    unsafe { panic_impl(fmt, file, line, col) }
 }

src/libcore/panicking.rs

@@ -28,6 +28,7 @@ use type_of;
 use type_::Type;
 
 use syntax::symbol::Symbol;
+use syntax_pos::Pos;
 
 use std::cmp;
 
@@ -333,6 +334,7 @@ impl<'a, 'tcx> MirContext<'a, 'tcx> {
                 let filename = Symbol::intern(&loc.file.name).as_str();
                 let filename = C_str_slice(bcx.ccx, filename);
                 let line = C_u32(bcx.ccx, loc.line as u32);
+                let col = C_u32(bcx.ccx, loc.col.to_usize() as u32 + 1);
 
                 // Put together the arguments to the panic entry point.
                 let (lang_item, args, const_err) = match *msg {
@@ -347,29 +349,29 @@ impl<'a, 'tcx> MirContext<'a, 'tcx> {
                                     index: index as u64
                                 }));
 
-                        let file_line = C_struct(bcx.ccx, &[filename, line], false);
-                        let align = llalign_of_min(bcx.ccx, common::val_ty(file_line));
-                        let file_line = consts::addr_of(bcx.ccx,
-                                                        file_line,
-                                                        align,
-                                                        "panic_bounds_check_loc");
+                        let file_line_col = C_struct(bcx.ccx, &[filename, line, col], false);
+                        let align = llalign_of_min(bcx.ccx, common::val_ty(file_line_col));
+                        let file_line_col = consts::addr_of(bcx.ccx,
+                                                            file_line_col,
+                                                            align,
+                                                            "panic_bounds_check_loc");
                         (lang_items::PanicBoundsCheckFnLangItem,
-                         vec![file_line, index, len],
+                         vec![file_line_col, index, len],
                          const_err)
                     }
                     mir::AssertMessage::Math(ref err) => {
                         let msg_str = Symbol::intern(err.description()).as_str();
                         let msg_str = C_str_slice(bcx.ccx, msg_str);
-                        let msg_file_line = C_struct(bcx.ccx,
-                                                     &[msg_str, filename, line],
+                        let msg_file_line_col = C_struct(bcx.ccx,
+                                                     &[msg_str, filename, line, col],
                                                      false);
-                        let align = llalign_of_min(bcx.ccx, common::val_ty(msg_file_line));
-                        let msg_file_line = consts::addr_of(bcx.ccx,
-                                                            msg_file_line,
-                                                            align,
-                                                            "panic_loc");
+                        let align = llalign_of_min(bcx.ccx, common::val_ty(msg_file_line_col));
+                        let msg_file_line_col = consts::addr_of(bcx.ccx,
+                                                                msg_file_line_col,
+                                                                align,
+                                                                "panic_loc");
                         (lang_items::PanicFnLangItem,
-                         vec![msg_file_line],
+                         vec![msg_file_line_col],
                          Some(ErrKind::Math(err.clone())))
                     }
                 };

src/librustc_trans/mir/block.rs

@@ -41,10 +41,10 @@ macro_rules! panic {
         panic!("explicit panic")
     });
     ($msg:expr) => ({
-        $crate::rt::begin_panic($msg, {
+        $crate::rt::begin_panic_new($msg, {
             // static requires less code at runtime, more constant data
-            static _FILE_LINE: (&'static str, u32) = (file!(), line!());
-            &_FILE_LINE
+            static _FILE_LINE_COL: (&'static str, u32, u32) = (file!(), line!(), column!());
+            &_FILE_LINE_COL
         })
     });
     ($fmt:expr, $($arg:tt)+) => ({
@@ -53,8 +53,8 @@ macro_rules! panic {
             // used inside a dead function. Just `#[allow(dead_code)]` is
             // insufficient, since the user may have
             // `#[forbid(dead_code)]` and which cannot be overridden.
-            static _FILE_LINE: (&'static str, u32) = (file!(), line!());
-            &_FILE_LINE
+            static _FILE_LINE_COL: (&'static str, u32, u32) = (file!(), line!(), column!());
+            &_FILE_LINE_COL
         })
     });
 }

src/libstd/macros.rs

@@ -262,6 +262,7 @@ impl<'a> PanicInfo<'a> {
 pub struct Location<'a> {
     file: &'a str,
     line: u32,
+    col: u32,
 }
 
 impl<'a> Location<'a> {
@@ -308,6 +309,29 @@ impl<'a> Location<'a> {
     pub fn line(&self) -> u32 {
         self.line
     }
+
+    /// Returns the column from which the panic originated.
+    ///
+    /// # Examples
+    ///
+    /// ```should_panic
+    /// #![feature(panic_col)]
+    /// use std::panic;
+    ///
+    /// panic::set_hook(Box::new(|panic_info| {
+    ///     if let Some(location) = panic_info.location() {
+    ///         println!("panic occured at column {}", location.column());
+    ///     } else {
+    ///         println!("panic occured but can't get location information...");
+    ///     }
+    /// }));
+    ///
+    /// panic!("Normal panic");
+    /// ```
+    #[unstable(feature = "panic_col", reason = "recently added", issue = "42939")]
+    pub fn column(&self) -> u32 {
+        self.col
+    }
 }
 
 fn default_hook(info: &PanicInfo) {
@@ -329,6 +353,7 @@ fn default_hook(info: &PanicInfo) {
 
     let file = info.location.file;
     let line = info.location.line;
+    let col = info.location.col;
 
     let msg = match info.payload.downcast_ref::<&'static str>() {
         Some(s) => *s,
@@ -342,8 +367,8 @@ fn default_hook(info: &PanicInfo) {
     let name = thread.as_ref().and_then(|t| t.name()).unwrap_or("<unnamed>");
 
     let write = |err: &mut ::io::Write| {
-        let _ = writeln!(err, "thread '{}' panicked at '{}', {}:{}",
-                         name, msg, file, line);
+        let _ = writeln!(err, "thread '{}' panicked at '{}', {}:{}:{}",
+                         name, msg, file, line, col);
 
         #[cfg(feature = "backtrace")]
         {
@@ -467,8 +492,9 @@ pub fn panicking() -> bool {
 #[unwind]
 pub extern fn rust_begin_panic(msg: fmt::Arguments,
                                file: &'static str,
-                               line: u32) -> ! {
-    begin_panic_fmt(&msg, &(file, line))
+                               line: u32,
+                               col: u32) -> ! {
+    begin_panic_fmt(&msg, &(file, line, col))
 }
 
 /// The entry point for panicking with a formatted message.
@@ -482,7 +508,7 @@ pub extern fn rust_begin_panic(msg: fmt::Arguments,
            issue = "0")]
 #[inline(never)] #[cold]
 pub fn begin_panic_fmt(msg: &fmt::Arguments,
-                       file_line: &(&'static str, u32)) -> ! {
+                       file_line_col: &(&'static str, u32, u32)) -> ! {
     use fmt::Write;
 
     // We do two allocations here, unfortunately. But (a) they're
@@ -492,7 +518,39 @@ pub fn begin_panic_fmt(msg: &fmt::Arguments,
 
     let mut s = String::new();
     let _ = s.write_fmt(*msg);
-    begin_panic(s, file_line)
+    begin_panic_new(s, file_line_col)
+}
+
+// FIXME: In PR #42938, we have added the column as info passed to the panic
+// handling code. For this, we want to break the ABI of begin_panic.
+// This is not possible to do directly, as the stage0 compiler is hardcoded
+// to emit a call to begin_panic in src/libsyntax/ext/build.rs, only
+// with the file and line number being passed, but not the colum number.
+// By changing the compiler source, we can only affect behaviour of higher
+// stages. We need to perform the switch over two stage0 replacements, using
+// a temporary function begin_panic_new while performing the switch:
+// 0. Right now, we tell stage1 onward to emit a call to begin_panic_new.
+// 1. In the first SNAP, stage0 calls begin_panic_new with the new ABI,
+//    begin_panic stops being used. Now we can change begin_panic to
+//    the new ABI, and start emitting calls to begin_panic in higher
+//    stages again, this time with the new ABI.
+// 2. After the second SNAP, stage0 calls begin_panic with the new ABI,
+//    and we can remove the temporary begin_panic_new function.
+
+/// This is the entry point of panicking for panic!() and assert!().
+#[unstable(feature = "libstd_sys_internals",
+           reason = "used by the panic! macro",
+           issue = "0")]
+#[inline(never)] #[cold] // avoid code bloat at the call sites as much as possible
+pub fn begin_panic_new<M: Any + Send>(msg: M, file_line_col: &(&'static str, u32, u32)) -> ! {
+    // Note that this should be the only allocation performed in this code path.
+    // Currently this means that panic!() on OOM will invoke this code path,
+    // but then again we're not really ready for panic on OOM anyway. If
+    // we do start doing this, then we should propagate this allocation to
+    // be performed in the parent of this thread instead of the thread that's
+    // panicking.
+
+    rust_panic_with_hook(Box::new(msg), file_line_col)
 }
 
 /// This is the entry point of panicking for panic!() and assert!().
@@ -508,7 +566,10 @@ pub fn begin_panic<M: Any + Send>(msg: M, file_line: &(&'static str, u32)) -> !
     // be performed in the parent of this thread instead of the thread that's
     // panicking.
 
-    rust_panic_with_hook(Box::new(msg), file_line)
+    let (file, line) = *file_line;
+    let file_line_col = (file, line, 0);
+
+    rust_panic_with_hook(Box::new(msg), &file_line_col)
 }
 
 /// Executes the primary logic for a panic, including checking for recursive
@@ -520,8 +581,8 @@ pub fn begin_panic<M: Any + Send>(msg: M, file_line: &(&'static str, u32)) -> !
 #[inline(never)]
 #[cold]
 fn rust_panic_with_hook(msg: Box<Any + Send>,
-                        file_line: &(&'static str, u32)) -> ! {
-    let (file, line) = *file_line;
+                        file_line_col: &(&'static str, u32, u32)) -> ! {
+    let (file, line, col) = *file_line_col;
 
     let panics = update_panic_count(1);
 
@@ -540,8 +601,9 @@ fn rust_panic_with_hook(msg: Box<Any + Send>,
         let info = PanicInfo {
             payload: &*msg,
             location: Location {
-                file: file,
-                line: line,
+                file,
+                line,
+                col,
             },
         };
         HOOK_LOCK.read();

src/libstd/panicking.rs

@@ -25,7 +25,7 @@
 
 
 // Reexport some of our utilities which are expected by other crates.
-pub use panicking::{begin_panic, begin_panic_fmt, update_panic_count};
+pub use panicking::{begin_panic_new, begin_panic, begin_panic_fmt, update_panic_count};
 
 #[cfg(not(test))]
 #[lang = "start"]