Skip to content

Commit

Permalink
Auto merge of #82271 - Aaron1011:debug-refcell, r=m-ou-se
Browse files Browse the repository at this point in the history
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
```
  • Loading branch information
bors committed Mar 23, 2021
2 parents 2bd94f4 + a23273e commit 9b6339e
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 11 deletions.
3 changes: 3 additions & 0 deletions library/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 = []
84 changes: 73 additions & 11 deletions library/core/src/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,19 +575,32 @@ 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>,
}

/// An error returned by [`RefCell::try_borrow`].
#[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()
}
}

Expand All @@ -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()
}
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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(),
}),
}
}

Expand Down Expand Up @@ -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(),
}),
}
}

Expand Down Expand Up @@ -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(),
})
}
}
}
Expand Down

0 comments on commit 9b6339e

Please sign in to comment.