Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debug-refcell feature to libcore #82271

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Conversation

Aaron1011
Copy link
Member

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:

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

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2021
@Aaron1011 Aaron1011 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Feb 18, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

library/core/src/cell.rs Outdated Show resolved Hide resolved
library/core/src/cell.rs Outdated Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member

Implementation seems reasonable - I'm not sure how much we want to do this, but I could easily see it being quite useful in practice.

r? @m-ou-se for T-libs

@Aaron1011
Copy link
Member Author

Ancedotally, I've already used this to fix a bug (ruffle-rs/ruffle#3322) where the borrow() and borrow_mut() calls were very far apart in the call stack.

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Why not add the location to the BorrowError and BorrowMutError types instead? Then you don't need to modify the borrow() and borrow_mut() functions at all, as the .expect() will include the information in the panic message. And it'll make try_borrow().unwrap() (etc.) also show the information.

library/core/Cargo.toml Show resolved Hide resolved
library/core/src/cell.rs Outdated Show resolved Hide resolved
library/core/src/cell.rs Outdated Show resolved Hide resolved
@Aaron1011
Copy link
Member Author

@m-ou-se I've addressed your comments. I've only modified the Debug impls of BorrowError/BorrowMutError - I've left the Display impls alone for now.

@rust-log-analyzer

This comment has been minimized.

@Aaron1011
Copy link
Member Author

Aaron1011 commented Mar 13, 2021

The assertion failure is due to the fact that I've enabled this by default to make testing easier. I'll disable the feature by default before this is merged.

Copy link
Member

@m-ou-se m-ou-se left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

r=me with the feature disabled by default.

library/core/src/cell.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

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
```
@Aaron1011
Copy link
Member Author

@bors r=m-ou-se

@bors
Copy link
Contributor

bors commented Mar 22, 2021

📌 Commit a23273e has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 22, 2021
@bors
Copy link
Contributor

bors commented Mar 23, 2021

⌛ Testing commit a23273e with merge 9b6339e...

@bors
Copy link
Contributor

bors commented Mar 23, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 9b6339e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 23, 2021
@bors bors merged commit 9b6339e into rust-lang:master Mar 23, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants