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

Compiler panics when disk is full #115298

Closed
DemiMarie opened this issue Aug 27, 2023 · 6 comments · Fixed by #115542
Closed

Compiler panics when disk is full #115298

DemiMarie opened this issue Aug 27, 2023 · 6 comments · Fixed by #115542
Assignees
Labels
C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@DemiMarie
Copy link
Contributor

When compiling my project, the disk filled up and I got:

thread 'rustc' panicked at 'assertion failed: pos.get() <= self.position()', compiler/rustc_metadata/src/rmeta/encoder.rs:447:9

I’m assuming this is a compiler bug, if only because the error message is not helpful. I haven’t included an example because I don’t think the specific code being compiled matters here.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 27, 2023
@workingjubilee workingjubilee added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 28, 2023
@Noratrieb Noratrieb added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ C-bug Category: This is a bug. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 29, 2023
@leikang123

This comment was marked as off-topic.

@ChrisDenton
Copy link
Member

Posting a wall of AI generated text is very unhelpful here. Please refrain from doing so in the future.

@saethlin
Copy link
Member

saethlin commented Sep 4, 2023

This ICE has been reported at least 9 times now; the issues above contain a total of 7 reports, then there's this issue and @kpreid posted about running into this on the community Discord.

I do not know why we've seen a recent uptick in reports of this ICE, but as far as I can tell the root cause is this PR (which predates all the reports): #94732, which subtly changes the API of Encoder. All writes are infallible but do not necessarily update the encoder position and thus any assertion which is based on inspecting the encoder position will explode if an encoder encounters an IO error.

It looks to me like this tripping hazard wasn't noticed at the time, which is hardly surprising considering that this is along an error path and all the reports we have will result in unsuccessful compilation even if the assertion isn't tripped.

I am going to try to make this not ICE anymore. The current implementation of FileEncoder has a reimplementation of BufWriter woven together with the delayed error handling; in its current state I have little confidence in my ability to review a fix that just adds updates to the encoder position so I'm going to try to untangle the logic first.

@saethlin saethlin self-assigned this Sep 4, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 11, 2023
…=<try>

Simplify/Optimize FileEncoder

FileEncoder is basically a BufWriter except that it exposes access to the not-written-to-yet region of the buffer so that some users can write directly to the buffer. This strategy is awesome because it lets us avoid calling memcpy for small copies, but the previous strategy was based on the writer accessing a `&mut [MaybeUninit<u8>; N]` and returning a `&[u8]` which is an API which currently mandates the use of unsafe code, making that interface in general not that appealing.

So this PR cleans up the FileEncoder implementation and builds on that general idea of direct buffer access in order to prevent `memcpy` calls in a few key places when encoding the dep graph and rmeta tables. The interface used here is now 100% safe, but with the caveat that internally we need to avoid trusting the number of bytes that the provided function claims to have written.

The original primary objective of this PR was to clean up the FileEncoder implementation so that the fix for the following issues would be easy to implement. The fix for these issues is to correctly update self.buffered even when writes fail, which I think it's easy to verify manually is now done, because all the FileEncoder methods are now small.

Fixes rust-lang#115298
Fixes rust-lang#114671
Fixes rust-lang#108100
Fixes rust-lang#106787
@bors bors closed this as completed in 3223b0b Sep 20, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 21, 2023
…pkin

Simplify/Optimize FileEncoder

FileEncoder is basically a BufWriter except that it exposes access to the not-written-to-yet region of the buffer so that some users can write directly to the buffer. This strategy is awesome because it lets us avoid calling memcpy for small copies, but the previous strategy was based on the writer accessing a `&mut [MaybeUninit<u8>; N]` and returning a `&[u8]` which is an API which currently mandates the use of unsafe code, making that interface in general not that appealing.

So this PR cleans up the FileEncoder implementation and builds on that general idea of direct buffer access in order to prevent `memcpy` calls in a few key places when encoding the dep graph and rmeta tables. The interface used here is now 100% safe, but with the caveat that internally we need to avoid trusting the number of bytes that the provided function claims to have written.

The original primary objective of this PR was to clean up the FileEncoder implementation so that the fix for the following issues would be easy to implement. The fix for these issues is to correctly update self.buffered even when writes fail, which I think it's easy to verify manually is now done, because all the FileEncoder methods are small.

Fixes rust-lang/rust#115298
Fixes rust-lang/rust#114671
Fixes rust-lang/rust#114045
Fixes rust-lang/rust#108100
Fixes rust-lang/rust#106787
@felipelalli
Copy link

felipelalli commented Sep 21, 2023

It might be good, in addition to fixing this specific error, for the compiler to estimate a "safe" disk space requirement before starting its operations. It could at least issue a warning before initiating or completely fail and proceed only if the user decides to force it.

It could also issue a warning at the end of a compilation when disk space is running low, something like:

"WARN: Low disk space, this may affect future compilations."

@saethlin
Copy link
Member

You're welcome to open a feature request issue for this.

I do not think we can do such estimation accurately enough to have an acceptable false positive rate, so I suspect we would get reports of the warning firing when it shouldn't when people compile on systems with a small disk. If we actually issue such a thing as a warning, we will probably break a lot of CI that uses -Dwarnings due to estimation error.

@felipelalli
Copy link

You're welcome to open a feature request issue for this.

I do not think we can do such estimation accurately enough to have an acceptable false positive rate, so I suspect we would get reports of the warning firing when it shouldn't when people compile on systems with a small disk. If we actually issue such a thing as a warning, we will probably break a lot of CI that uses -Dwarnings due to estimation error.

Well, I suggested it. As expected, the idea was poorly received. Here is the link for record and cross-reference: Check disk space before compilation.

lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…pkin

Simplify/Optimize FileEncoder

FileEncoder is basically a BufWriter except that it exposes access to the not-written-to-yet region of the buffer so that some users can write directly to the buffer. This strategy is awesome because it lets us avoid calling memcpy for small copies, but the previous strategy was based on the writer accessing a `&mut [MaybeUninit<u8>; N]` and returning a `&[u8]` which is an API which currently mandates the use of unsafe code, making that interface in general not that appealing.

So this PR cleans up the FileEncoder implementation and builds on that general idea of direct buffer access in order to prevent `memcpy` calls in a few key places when encoding the dep graph and rmeta tables. The interface used here is now 100% safe, but with the caveat that internally we need to avoid trusting the number of bytes that the provided function claims to have written.

The original primary objective of this PR was to clean up the FileEncoder implementation so that the fix for the following issues would be easy to implement. The fix for these issues is to correctly update self.buffered even when writes fail, which I think it's easy to verify manually is now done, because all the FileEncoder methods are small.

Fixes rust-lang/rust#115298
Fixes rust-lang/rust#114671
Fixes rust-lang/rust#114045
Fixes rust-lang/rust#108100
Fixes rust-lang/rust#106787
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…pkin

Simplify/Optimize FileEncoder

FileEncoder is basically a BufWriter except that it exposes access to the not-written-to-yet region of the buffer so that some users can write directly to the buffer. This strategy is awesome because it lets us avoid calling memcpy for small copies, but the previous strategy was based on the writer accessing a `&mut [MaybeUninit<u8>; N]` and returning a `&[u8]` which is an API which currently mandates the use of unsafe code, making that interface in general not that appealing.

So this PR cleans up the FileEncoder implementation and builds on that general idea of direct buffer access in order to prevent `memcpy` calls in a few key places when encoding the dep graph and rmeta tables. The interface used here is now 100% safe, but with the caveat that internally we need to avoid trusting the number of bytes that the provided function claims to have written.

The original primary objective of this PR was to clean up the FileEncoder implementation so that the fix for the following issues would be easy to implement. The fix for these issues is to correctly update self.buffered even when writes fail, which I think it's easy to verify manually is now done, because all the FileEncoder methods are small.

Fixes rust-lang/rust#115298
Fixes rust-lang/rust#114671
Fixes rust-lang/rust#114045
Fixes rust-lang/rust#108100
Fixes rust-lang/rust#106787
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants