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.

Sign up for GitHub

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

Jump to bottom

FileEncoder delayed error reporting is still broken #117254

Closed
saethlin opened this issue Oct 27, 2023 · 0 comments · Fixed by #117301
Closed

FileEncoder delayed error reporting is still broken #117254

saethlin opened this issue Oct 27, 2023 · 0 comments · Fixed by #117301
Assignees
@saethlin
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

@saethlin
Copy link
Member

#94732 introduced a delayed error reporting scheme which was either buggy when it was merged or became buggy shortly after. The bug only surfaced as a variety of ICEs upon running out of disk space. In #115542 I tried to fix the problem by simplifying FileEncoder so that it was harder to lose track of the delayed error.

But it looks like somehow this is still broken.

The failures I'm looking at are all in this crater run: #104862 (comment) and they all look like https://crater-reports.s3.amazonaws.com/pr-104862/master%237db4a89d49a8ed3a5f79b6cc3d555696baa1bbc3/gh/LooMaclin.test_kafka_musl/log.txt

[INFO] [stderr]    Compiling serde_json v1.0.39
[INFO] [stderr] thread 'rustc' panicked at /rustc/7db4a89d49a8ed3a5f79b6cc3d555696baa1bbc3/compiler/rustc_serialize/src/opaque.rs:233:42:
[INFO] [stderr] LLVM ERROR: IO failure on output stream: No space left on device
[INFO] [stderr] error: could not compile `serde` (lib)

We are ICEing in MemDecoder now. The delayed error reporting is supposed to report errors and halt the built when we try to finalize writing out the file where we ran into an issue, but it looks like that isn't happening. We're going ahead to decode a result which was not completely written out, and we try to access at an offset beyond the end of the mmap'd file.
@saethlin saethlin added 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. C-bug Category: This is a bug. labels Oct 27, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 27, 2023
@saethlin saethlin self-assigned this Oct 27, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 27, 2023
@saethlin saethlin mentioned this issue Oct 28, 2023
Merged
@bors bors closed this as completed in 3dbb4da Nov 26, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Nov 27, 2023
@bors
Auto merge of #117301 - saethlin:finish-rmeta-encoding, r=WaffleLapkin
ca3dcde 
Call FileEncoder::finish in rmeta encoding

Fixes rust-lang/rust#117254

The bug here was that rmeta encoding never called FileEncoder::finish. Now it does. Most of the changes here are needed to support that, since rmeta encoding wants to finish _then_ access the File in the encoder, so finish can't move out.

I tried adding a `cfg(debug_assertions)` exploding Drop impl to FileEncoder that checked for finish being called before dropping, but fatal errors cause unwinding so this isn't really possible. If we encounter a fatal error with a dirty FileEncoder, the Drop impl ICEs even though the implementation is correct. If we try to paper over that by wrapping FileEncoder in ManuallyDrop then that just erases the fact that Drop automatically checks that we call finish on all paths.

I also changed the name of DepGraph::encode to DepGraph::finish_encoding, because that's what it does and it makes the fact that it is the path to FileEncoder::finish less confusing.

r? `@WaffleLapkin`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@saethlin saethlin

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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Call FileEncoder::finish in rmeta encoding saethlin/rust
2 participants
@saethlin @rustbot