Skip to content

Commit a23273e

Browse files
committed
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 { location: Location { file: "refcell_test.rs", line: 6, col: 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 { location: Location { file: "refcell_test.rs", line: 16, col: 27 } }', refcell_test.rs:18:25 ```
1 parent d04c3aa commit a23273e

File tree

2 files changed

+76
-11
lines changed

2 files changed

+76
-11
lines changed

library/core/Cargo.toml

+3
Original file line numberDiff line numberDiff line change
@@ -25,3 +25,6 @@ rand = "0.7"
2525
[features]
2626
# Make panics and failed asserts immediately abort without formatting any message
2727
panic_immediate_abort = []
28+
# Make `RefCell` store additional debugging information, which is printed out when
29+
# a borrow error occurs
30+
debug_refcell = []

library/core/src/cell.rs

+73-11
Original file line numberDiff line numberDiff line change
@@ -575,19 +575,32 @@ impl<T> Cell<[T]> {
575575
#[stable(feature = "rust1", since = "1.0.0")]
576576
pub struct RefCell<T: ?Sized> {
577577
borrow: Cell<BorrowFlag>,
578+
// Stores the location of the earliest currently active borrow.
579+
// This gets updated whenver we go from having zero borrows
580+
// to having a single borrow. When a borrow occurs, this gets included
581+
// in the generated `BorroeError/`BorrowMutError`
582+
#[cfg(feature = "debug_refcell")]
583+
borrowed_at: Cell<Option<&'static crate::panic::Location<'static>>>,
578584
value: UnsafeCell<T>,
579585
}
580586

581587
/// An error returned by [`RefCell::try_borrow`].
582588
#[stable(feature = "try_borrow", since = "1.13.0")]
583589
pub struct BorrowError {
584590
_private: (),
591+
#[cfg(feature = "debug_refcell")]
592+
location: &'static crate::panic::Location<'static>,
585593
}
586594

587595
#[stable(feature = "try_borrow", since = "1.13.0")]
588596
impl Debug for BorrowError {
589597
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
590-
f.debug_struct("BorrowError").finish()
598+
let mut builder = f.debug_struct("BorrowError");
599+
600+
#[cfg(feature = "debug_refcell")]
601+
builder.field("location", self.location);
602+
603+
builder.finish()
591604
}
592605
}
593606

@@ -602,12 +615,19 @@ impl Display for BorrowError {
602615
#[stable(feature = "try_borrow", since = "1.13.0")]
603616
pub struct BorrowMutError {
604617
_private: (),
618+
#[cfg(feature = "debug_refcell")]
619+
location: &'static crate::panic::Location<'static>,
605620
}
606621

607622
#[stable(feature = "try_borrow", since = "1.13.0")]
608623
impl Debug for BorrowMutError {
609624
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
610-
f.debug_struct("BorrowMutError").finish()
625+
let mut builder = f.debug_struct("BorrowMutError");
626+
627+
#[cfg(feature = "debug_refcell")]
628+
builder.field("location", self.location);
629+
630+
builder.finish()
611631
}
612632
}
613633

@@ -658,7 +678,12 @@ impl<T> RefCell<T> {
658678
#[rustc_const_stable(feature = "const_refcell_new", since = "1.24.0")]
659679
#[inline]
660680
pub const fn new(value: T) -> RefCell<T> {
661-
RefCell { value: UnsafeCell::new(value), borrow: Cell::new(UNUSED) }
681+
RefCell {
682+
value: UnsafeCell::new(value),
683+
borrow: Cell::new(UNUSED),
684+
#[cfg(feature = "debug_refcell")]
685+
borrowed_at: Cell::new(None),
686+
}
662687
}
663688

664689
/// Consumes the `RefCell`, returning the wrapped value.
@@ -823,12 +848,29 @@ impl<T: ?Sized> RefCell<T> {
823848
/// ```
824849
#[stable(feature = "try_borrow", since = "1.13.0")]
825850
#[inline]
851+
#[cfg_attr(feature = "debug_refcell", track_caller)]
826852
pub fn try_borrow(&self) -> Result<Ref<'_, T>, BorrowError> {
827853
match BorrowRef::new(&self.borrow) {
828-
// SAFETY: `BorrowRef` ensures that there is only immutable access
829-
// to the value while borrowed.
830-
Some(b) => Ok(Ref { value: unsafe { &*self.value.get() }, borrow: b }),
831-
None => Err(BorrowError { _private: () }),
854+
Some(b) => {
855+
#[cfg(feature = "debug_refcell")]
856+
{
857+
// `borrowed_at` is always the *first* active borrow
858+
if b.borrow.get() == 1 {
859+
self.borrowed_at.set(Some(crate::panic::Location::caller()));
860+
}
861+
}
862+
863+
// SAFETY: `BorrowRef` ensures that there is only immutable access
864+
// to the value while borrowed.
865+
Ok(Ref { value: unsafe { &*self.value.get() }, borrow: b })
866+
}
867+
None => Err(BorrowError {
868+
_private: (),
869+
// If a borrow occured, then we must already have an outstanding borrow,
870+
// so `borrowed_at` will be `Some`
871+
#[cfg(feature = "debug_refcell")]
872+
location: self.borrowed_at.get().unwrap(),
873+
}),
832874
}
833875
}
834876

@@ -896,11 +938,25 @@ impl<T: ?Sized> RefCell<T> {
896938
/// ```
897939
#[stable(feature = "try_borrow", since = "1.13.0")]
898940
#[inline]
941+
#[cfg_attr(feature = "debug_refcell", track_caller)]
899942
pub fn try_borrow_mut(&self) -> Result<RefMut<'_, T>, BorrowMutError> {
900943
match BorrowRefMut::new(&self.borrow) {
901-
// SAFETY: `BorrowRef` guarantees unique access.
902-
Some(b) => Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b }),
903-
None => Err(BorrowMutError { _private: () }),
944+
Some(b) => {
945+
#[cfg(feature = "debug_refcell")]
946+
{
947+
self.borrowed_at.set(Some(crate::panic::Location::caller()));
948+
}
949+
950+
// SAFETY: `BorrowRef` guarantees unique access.
951+
Ok(RefMut { value: unsafe { &mut *self.value.get() }, borrow: b })
952+
}
953+
None => Err(BorrowMutError {
954+
_private: (),
955+
// If a borrow occured, then we must already have an outstanding borrow,
956+
// so `borrowed_at` will be `Some`
957+
#[cfg(feature = "debug_refcell")]
958+
location: self.borrowed_at.get().unwrap(),
959+
}),
904960
}
905961
}
906962

@@ -1016,7 +1072,13 @@ impl<T: ?Sized> RefCell<T> {
10161072
// and is thus guaranteed to be valid for the lifetime of `self`.
10171073
Ok(unsafe { &*self.value.get() })
10181074
} else {
1019-
Err(BorrowError { _private: () })
1075+
Err(BorrowError {
1076+
_private: (),
1077+
// If a borrow occured, then we must already have an outstanding borrow,
1078+
// so `borrowed_at` will be `Some`
1079+
#[cfg(feature = "debug_refcell")]
1080+
location: self.borrowed_at.get().unwrap(),
1081+
})
10201082
}
10211083
}
10221084
}

0 commit comments

Comments
 (0)