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

Avoid BorrowMutError with RUSTC_LOG=debug #78524

Merged
merged 4 commits into from
Oct 30, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Oct 29, 2020

$ touch empty.rs
$ env RUSTC_LOG=debug rustc +stage1 --crate-type=lib empty.rs

Fails with a BorrowMutError because source map files are already
borrowed while features_query attempts to format a log message
containing a span.

Release the borrow before the query to avoid the issue.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2020
@Aaron1011
Copy link
Member

It looks like neither of these uses of SourceMap::files actually needs write access.

@tmiasko: Could you try changing SourceMap.files to a rustc_data_structures::sync::RwLock? That should prevent this kind of issue from coming up again, since there are lots of places that try to print a Span. This might also improve concurrency in the parallel compiler, since the overwhelming majority of accesses to the source map files should be reads.

@tmiasko tmiasko force-pushed the source-files-borrow branch from 3af65d4 to bd3ddff Compare October 29, 2020 15:55
@jyn514 jyn514 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 29, 2020
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 29, 2020

Replacing Lock with RwLocks works as well, I tried that earlier. Shortening the borrow of source files seems like a good idea regardless, so I left those changes.

@Aaron1011
Copy link
Member

@tmiasko: Can you add a regression test? I would use src/test/ui/rustc-rust-log.rs as a starting point, since we don't actually want to record the log output.

$ touch empty.rs
$ env RUSTC_LOG=debug rustc +stage1 --crate-type=lib empty.rs

Fails with a `BorrowMutError` because source map files are already
borrowed while `features_query` attempts to format a log message
containing a span.

Release the borrow before the query to avoid the issue.
@tmiasko tmiasko force-pushed the source-files-borrow branch from bd3ddff to a15e0dc Compare October 29, 2020 17:29
@tmiasko
Copy link
Contributor Author

tmiasko commented Oct 29, 2020

Extended rustc-rust-log.rs with an aux-build that uses RUSTC_LOG=debug during build.

@Aaron1011
Copy link
Member

Aaron1011 commented Oct 29, 2020

LGTM. The compiletest changes look reasonable to me, but I'd like to get someone else to sign off on them as well.

cc @alexcrichton: It looks like you've reviewed compiletest changes before.

@alexcrichton
Copy link
Member

Seem reasonable to me as well!

@Aaron1011
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 29, 2020

📌 Commit a15e0dc has been approved by Aaron1011

@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 Oct 29, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 30, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77334 (Reorder benches const variable)
 - rust-lang#77888 (Simplify a nested bool match)
 - rust-lang#77921 (f64: Refactor collapsible_if)
 - rust-lang#78523 (Revert invalid `fn` return type parsing change)
 - rust-lang#78524 (Avoid BorrowMutError with RUSTC_LOG=debug)
 - rust-lang#78545 (Make anonymous binders start at 0)
 - rust-lang#78554 (Improve wording of `core::ptr::drop_in_place` docs)
 - rust-lang#78556 (Link to pass docs from NRVO module docs)

Failed merges:

 - rust-lang#78424 (Fix some more clippy warnings)

r? `@ghost`
@bors bors merged commit f853922 into rust-lang:master Oct 30, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 30, 2020
@tmiasko tmiasko deleted the source-files-borrow branch October 30, 2020 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants