diff options
| author | bors <bors@rust-lang.org> | 2021-03-23 04:49:47 +0000 |
|---|---|---|
| committer | bors <bors@rust-lang.org> | 2021-03-23 04:49:47 +0000 |
| commit | 9b6339e4b9747d473270baa42e77e1d2fff39bf4 (patch) | |
| tree | e96ec38118428561cd1eb0ac653e065b8de4d686 | |
| parent | 2bd94f4aa9683158c7df96b9661be193c39a55ed (diff) | |
| parent | a23273e82c8156974553e43f7d61826651397fbc (diff) | |
| download | rust-9b6339e4b9747d473270baa42e77e1d2fff39bf4.tar.gz rust-9b6339e4b9747d473270baa42e77e1d2fff39bf4.zip | |
Auto merge of #82271 - Aaron1011:debug-refcell, r=m-ou-se
Add `debug-refcell` feature to libcore
See https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Attaching.20backtraces.20to.20RefCell/near/226273614
for some background discussion
This PR adds a new off-by-default feature `debug-refcell` to libcore.
When enabled, this feature stores additional debugging information in
`RefCell`. This information is included in the panic message when
`borrow()` or `borrow_mut()` panics, to make it easier to track down the
source of the issue.
Currently, we store the caller location for the earliest active borrow.
This has a number of advantages:
* There is only a constant amount of overhead per `RefCell`
* We don't need any heap memory, so it can easily be implemented in core
* Since we are storing the *earliest* active borrow, we don't need any
extra logic in the `Drop` implementation for `Ref` and `RefMut`
Limitations:
* We only store the caller location, not a full `Backtrace`. Until
we get support for `Backtrace` in libcore, this is the best tha we can
do.
* The captured location is only displayed when `borrow()` or
`borrow_mut()` panics. If a crate calls `try_borrow().unwrap()`
or `try_borrow_mut().unwrap()`, this extra information will be lost.
To make testing easier, I've enabled the `debug-refcell` feature by
default. I'm not sure how to write a test for this feature - we would
need to rebuild core from the test framework, and create a separate
sysroot.
Since this feature will be off-by-default, users will need to use
`xargo` or `cargo -Z build-std` to enable this feature. For users using
a prebuilt standard library, this feature will be disabled with zero
overhead.
I've created a simple test program:
```rust
use std::cell::RefCell;
fn main() {
let _ = std::panic::catch_unwind(|| {
let val = RefCell::new(true);
let _first = val.borrow();
let _second = val.borrow();
let _third = val.borrow_mut();
});
let _ = std::panic::catch_unwind(|| {
let val = RefCell::new(true);
let first = val.borrow_mut();
drop(first);
let _second = val.borrow_mut();
let _thid = val.borrow();
});
}
```
which produces the following output:
```
thread 'main' panicked at 'already borrowed: BorrowMutError at refcell_test.rs:6:26', refcell_test.rs:8:26
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'already mutably borrowed: BorrowError at refcell_test.rs:16:27', refcell_test.rs:18:25
```
| -rw-r--r-- | library/core/Cargo.toml | 3 | ||||
| -rw-r--r-- | library/core/src/cell.rs | 84 |
2 files changed, 76 insertions, 11 deletions
diff --git a/library/core/Cargo.toml b/library/core/Cargo.toml index 4a7dbb91822..b6aaf078cf0 100644 --- a/library/core/Cargo.toml +++ b/library/core/Cargo.toml @@ -28,3 +28,6 @@ rand = "0.7" [features] # Make panics and failed asserts immediately abort without formatting any message panic_immediate_abort = [] +# Make `RefCell` store additional debugging information, which is printed out when +# a borrow error occurs +debug_refcell = [] diff --git a/library/core/src/cell.rs b/library/core/src/cell.rs index fa21a40e169..770169219aa 100644 --- a/library/core/src/cell.rs +++ b/library/core/src/cell.rs @@ -575,6 +575,12 @@ impl<T> Cell<[T]> { #[stable(feature = "rust1", since = "1.0.0")] pub struct RefCell<T: ?Sized> { borrow: Cell<BorrowFlag>, + // Stores the location of the earliest currently active borrow. + // This gets updated whenver we go from having zero borrows + // to having a single borrow. When a borrow occurs, this gets included + // in the generated `BorroeError/`BorrowMutError` + #[cfg(feature = "debug_refcell")] + borrowed_at: Cell<Option<&'static crate::panic::Location<'static>>>, value: UnsafeCell<T>, } @@ -582,12 +588,19 @@ pub struct RefCell<T: ?Sized> { #[stable(feature = "try_borrow", since = "1.13.0")] pub struct BorrowError { _private: (), + #[cfg(feature = "debug_refcell")] + location: &'static crate::panic::Location<'static>, } #[stable(feature = "try_borrow", since = "1.13.0")] impl Debug for BorrowError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("BorrowError").finish() + let mut builder = f.debug_struct("BorrowError"); + + #[cfg(feature = "debug_refcell")] + builder.field("location", self.location); + + builder.finish() } } @@ -602,12 +615,19 @@ impl Display for BorrowError { #[stable(feature = "try_borrow", since = "1.13.0")] pub struct BorrowMutError { _private: (), + #[cfg(feature = "debug_refcell")] + location: &'static crate::panic::Location<'static>, } #[stable(feature = "try_borrow", since = "1.13.0")] impl Debug for BorrowMutError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("BorrowMutError").finish() + let mut builder = f.debug_struct("BorrowMutError"); + + #[cfg(feature = "debug_refcell")] + builder.field("location", self.location); + + builder.finish() } } @@ -658,7 +678,12 @@ impl<T> RefCell<T> { #[rustc_const_stable(feature = "const_refcell_new", since = "1.24.0")] #[inline] pub const fn new(value: T) -> RefCell<T> { - RefCell { value: UnsafeCell::new(value), borrow: Cell::new(UNUSED) } + RefCell { + value: UnsafeCell::new(value), + borrow: Cell::new(UNUSED), + #[cfg(feature = "debug_refcell")] + borrowed_at: Cell::new(None), + } } /// Consumes the `RefCell`, returning the wrapped value. @@ -823,12 +848,29 @@ impl<T: ?Sized> RefCell<T> { /// ``` #[stable(feature = "try_borrow", since = "1.13.0")] #[inline] + #[cfg_attr(feature = "debug_refcell", track_caller)] pub fn try_borrow(&self) -> Result<Ref<'_, T>, BorrowError> { match BorrowRef::new(&self.borrow) { - // SAFETY: `BorrowRef` ensures that there is only immutable access - // to the value while borrowed. - Some(b) => Ok(Ref { value: unsafe { &*self.value.get() }, borrow: b }), - None => Err(BorrowError { _private: () }), + Some(b) => { + #[cfg(feature = "debug_refcell")] + { + // `borrowed_at` is always the *first* active borrow + if b.borrow.get() == 1 { + self.borrowed_at.set(Some(crate::panic::Location::caller())); + } + } + + // SAFETY: `BorrowRef` ensures that there is only immutable access + // to the value while borrowed. + Ok(Ref { value: unsafe { &*self.value.get() }, borrow: b }) + } + None => Err(BorrowError { + _private: (), + // If a borrow occured, then we must already have an outstanding borrow, + // so `borrowed_at` will be `Some` + #[cfg(feature = "debug_refcell")] + location: self.borrowed_at.get().unwrap(), + }), } } @@ -896,11 +938,25 @@ impl<T: ?Sized> RefCell<T> { /// ``` #[stable(feature = "try_borrow", since = "1.13.0")] #[inline] + #[cfg_attr(feature = "debug_refcell", track_caller)] pub fn try_borrow_mut(&self) -> Result<RefMut<'_, T>, BorrowMutError> { match BorrowRefMut::new(&self.borrow) { - // SAFETY: `BorrowRef` guarantees unique access. - Some(b) => Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b }), - None => Err(BorrowMutError { _private: () }), + Some(b) => { + #[cfg(feature = "debug_refcell")] + { + self.borrowed_at.set(Some(crate::panic::Location::caller())); + } + + // SAFETY: `BorrowRef` guarantees unique access. + Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b }) + } + None => Err(BorrowMutError { + _private: (), + // If a borrow occured, then we must already have an outstanding borrow, + // so `borrowed_at` will be `Some` + #[cfg(feature = "debug_refcell")] + location: self.borrowed_at.get().unwrap(), + }), } } @@ -1016,7 +1072,13 @@ impl<T: ?Sized> RefCell<T> { // and is thus guaranteed to be valid for the lifetime of `self`. Ok(unsafe { &*self.value.get() }) } else { - Err(BorrowError { _private: () }) + Err(BorrowError { + _private: (), + // If a borrow occured, then we must already have an outstanding borrow, + // so `borrowed_at` will be `Some` + #[cfg(feature = "debug_refcell")] + location: self.borrowed_at.get().unwrap(), + }) } } } |
