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

Try to write the panic message with a single write_all call #122565

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 15, 2024

This writes the panic message to a buffer before writing to stderr. This allows it to be printed with a single write_all call, preventing it from being interleaved with other outputs. It also adds newlines before and after the message ensuring that only the panic message will have its own lines.

Before:

thread 'thread 'thread 'thread 'thread '<unnamed>thread 'thread 'thread 'thread '<unnamed><unnamed>thread '<unnamed>' panicked at ' panicked at <unnamed><unnamed><unnamed><unnamed><unnamed>' panicked at <unnamed>' panicked at src\heap.rssrc\heap.rs' 
panicked at ' panicked at ' panicked at ' panicked at ' panicked at src\heap.rs' panicked at src\heap.rs::src\heap.rssrc\heap.rssrc\heap.rssrc\heap.rssrc\heap.rs:src\heap.rs:455455:::::455:455::455455455455455:455:99:::::9:9:
:
999:
999:
assertion failed: size <= (*queue).block_size:
:
assertion failed: size <= (*queue).block_size:
assertion failed: size <= (*queue).block_size:
:
:
assertion failed: size <= (*queue).block_sizeassertion failed: size <= (*queue).block_sizeassertion failed: size <= (*queue).block_size

assertion failed: size <= (*queue).block_size
assertion failed: size <= (*queue).block_sizeassertion failed: size <= (*queue).block_sizeerror: process didn't exit successfully: `target\debug\direct_test.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

After:


thread '<unnamed>' panicked at src\heap.rs:455:9:
assertion failed: size <= (*queue).block_size


thread '<unnamed>' panicked at src\heap.rs:455:9:
assertion failed: size <= (*queue).block_size


thread '<unnamed>' panicked at src\heap.rs:455:9:
assertion failed: size <= (*queue).block_size

error: process didn't exit successfully: `target\debug\direct_test.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

try-jobs: x86_64-gnu-llvm-18

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 15, 2024
@rust-log-analyzer

This comment has been minimized.

@Zoxc Zoxc force-pushed the atomic-panic-msg branch from 139929a to e03a5d0 Compare March 15, 2024 20:13
@rust-log-analyzer

This comment has been minimized.

@Zoxc Zoxc force-pushed the atomic-panic-msg branch from e03a5d0 to f24ddfd Compare March 16, 2024 10:48
@rust-log-analyzer

This comment has been minimized.

@Zoxc Zoxc force-pushed the atomic-panic-msg branch from f24ddfd to 4962cfe Compare March 16, 2024 12:13
@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2024

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@Zoxc Zoxc force-pushed the atomic-panic-msg branch from 4962cfe to 0f7a671 Compare March 16, 2024 13:47
@rust-log-analyzer

This comment has been minimized.

@Zoxc Zoxc force-pushed the atomic-panic-msg branch from 0f7a671 to dd12eed Compare March 16, 2024 15:02
let mut cursor = crate::io::Cursor::new(&mut buffer[..]);

let write_msg = |dst: &mut dyn crate::io::Write| {
writeln!(dst, "\nthread '{name}' panicked at {location}:\n{msg}")
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to separate the functional change (more newlines) from the implementation change. Makes it easier to discuss and decide on these separately.

A leading newline is not very common in Rust output, so if it stays it definitely needs a comment explaining the purpose.

Copy link
Member

Choose a reason for hiding this comment

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

Panic output isn't stable. There's some work to add thread IDs to the panic message for example.

Yeah the extra newline is unusual, but this is an unsynchronized write to a shared output so we don't have many options to clean up the formatting in cases where a panic is emitted in the middle of something else that's printing.

proc_macro_generated_packed.stderr is a good example of this. It start printing the panic message in the middle of reporting another error.

Do you think it's disruptive enough that it warrants separate discussion?

@rustbot
Copy link
Collaborator

rustbot commented Mar 17, 2024

Some changes occurred in run-make tests.

cc @jieyouxu

@bors
Copy link
Contributor

bors commented Apr 2, 2024

☔ The latest upstream changes (presumably #122267) made this pull request unmergeable. Please resolve the merge conflicts.

@correabuscar
Copy link
Contributor

Would it be possible(or too overkill?) to check how much stack is left(but without using external crate) and if enough only then write to the on-stack buffer, else write to err directly to avoid a stack overflow ?

I sort of tried to look into it, preliminarily, and it seems that (perhaps due to println!? unsure) it needs >=816 bytes left on reported stack or it will stack overflow in this example:
https://github.com/correabuscar/sandbox/blob/9aecdfa0aead32bc37377a07e57030f623aaad7b/rust/05_sandbox/stack/how_much_stack_is_left/src/main.rs#L9

output is like this on linux/gentoo:

     Running `target/debug/how_much_stack_is_left`
Hello, main world! Some(8371024)
Hello, thread world! Some(19664)
Depth: 1, stack left: Some(19344)
Depth: 2, stack left: Some(19024)
...
Depth: 57, stack left: Some(1424)
Depth: 58, stack left: Some(1104)
End=58
From cloj, stack left: Some(816)

@workingjubilee
Copy link
Member

If we reserve a 512 byte buffer on the stack, then we don't have a guarantee that the compiler doesn't emit code that forces using 512 bytes deeper into the stack even on the codepath that does not write to that (as it may do something like, for instance, sub rsp, 512; push rax;).

@the8472 the8472 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2024
@correabuscar
Copy link
Contributor

If we reserve a 512 byte buffer on the stack, then we don't have a guarantee that the compiler doesn't emit code that forces using 512 bytes deeper into the stack even on the codepath that does not write to that (as it may do something like, for instance, sub rsp, 512; push rax;).

I'm not sure I understand that ^, which leads me to say this: but isn't the buffer only allocated on the stack if the specific closure gets to execute? assuming the buffer is inside that closure which we wouldn't execute in the case when we'd know there's not enough stack left for it.

@workingjubilee
Copy link
Member

workingjubilee commented May 17, 2024

I'm not sure I understand that ^, which leads me to say this: but isn't the buffer only allocated on the stack if the specific closure gets to execute? assuming the buffer is inside that closure which we wouldn't execute in the case when we'd know there's not enough stack left for it.

Speaking very broadly... please do not take this as a gospel fact in all cases, but rather as more "Jubilee's understanding of the underlying principles at work, as she has derived from experience (and sometimes talking to LLVM devs)"... LLVM does not view itself as beholden to concerns like "have we run out of stack?" when performing optimizations. That means it is basically always valid to hoist constant byte arrays like that to earlier.

That in itself wouldn't mean we allocate a bunch of extra stack always... but by and large, our closures tend to optimize well because they are inlined into the function that calls them, which means that the enclosing function and the closure are optimized together into a single call. We have to apply many annotations to discourage this behavior.

This doesn't mean LLVM loves optimizing byte arrays on the stack so much it will always do this, it's just that when it's about, like... 512 bytes? I don't think checking if we have 512 bytes available is worth the other costs.

@workingjubilee
Copy link
Member

on a less "what are legal program optimizations?" level: in general the stack is chunked into pages of 4KB or more, so using stacker to check for less than 4kb left of stack is... a little silly.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 29, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2024
Try to write the panic message with a single `write_all` call

This writes the panic message to a buffer before writing to stderr. This allows it to be printed with a single `write_all` call, preventing it from being interleaved with other outputs. It also adds newlines before and after the message ensuring that only the panic message will have its own lines.

Before:
```
thread 'thread 'thread 'thread 'thread '<unnamed>thread 'thread 'thread 'thread '<unnamed><unnamed>thread '<unnamed>' panicked at ' panicked at <unnamed><unnamed><unnamed><unnamed><unnamed>' panicked at <unnamed>' panicked at src\heap.rssrc\heap.rs'
panicked at ' panicked at ' panicked at ' panicked at ' panicked at src\heap.rs' panicked at src\heap.rs::src\heap.rssrc\heap.rssrc\heap.rssrc\heap.rssrc\heap.rs:src\heap.rs:455455:::::455:455::455455455455455:455:99:::::9:9:
:
999:
999:
assertion failed: size <= (*queue).block_size:
:
assertion failed: size <= (*queue).block_size:
assertion failed: size <= (*queue).block_size:
:
:
assertion failed: size <= (*queue).block_sizeassertion failed: size <= (*queue).block_sizeassertion failed: size <= (*queue).block_size

assertion failed: size <= (*queue).block_size
assertion failed: size <= (*queue).block_sizeassertion failed: size <= (*queue).block_sizeerror: process didn't exit successfully: `target\debug\direct_test.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)
```
After:
```

thread '<unnamed>' panicked at src\heap.rs:455:9:
assertion failed: size <= (*queue).block_size

thread '<unnamed>' panicked at src\heap.rs:455:9:
assertion failed: size <= (*queue).block_size

thread '<unnamed>' panicked at src\heap.rs:455:9:
assertion failed: size <= (*queue).block_size

error: process didn't exit successfully: `target\debug\direct_test.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)
```

---

try-jobs: x86_64-gnu-llvm-18
@bors
Copy link
Contributor

bors commented Dec 29, 2024

⌛ Testing commit bea996e with merge 315c630...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 29, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 29, 2024
weihanglo added a commit to weihanglo/cargo that referenced this pull request Dec 29, 2024
rust-lang/rust#122565 adds a new line to thread panic output.
To make the current test suites works on stable, beta, and nightly,
Similar to rust-lang#14602,
this relaxes the assertion around that by globbing everything.
weihanglo added a commit to weihanglo/cargo that referenced this pull request Dec 29, 2024
rust-lang/rust#122565 adds a new line to thread panic output.
To make the current test suites works on stable, beta, and nightly,
similar to rust-lang#14602,
this relaxes the assertion around that by globbing everything.
weihanglo added a commit to weihanglo/cargo that referenced this pull request Dec 29, 2024
rust-lang/rust#122565 adds a new line to thread panic output.
To make the current test suites works on stable, beta, and nightly,
similar to rust-lang#14602,
this relaxes the assertion around that by globbing everything.
@weihanglo
Copy link
Member

rust-lang/cargo#14989 is open for relaxing more assertions.

github-merge-queue bot pushed a commit to rust-lang/cargo that referenced this pull request Dec 30, 2024
### What does this PR try to resolve?

rust-lang/rust#122565 adds a new line to thread panic output.
To make the current test suites works on stable, beta, and nightly,
similar to #14602,
this relaxes the assertion around that by globbing everything.
@weihanglo weihanglo mentioned this pull request Dec 31, 2024
@the8472
Copy link
Member

the8472 commented Jan 2, 2025

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Jan 2, 2025

📌 Commit 4bf85c2 has been approved by the8472

It is now in the queue for this repository.

@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 Jan 2, 2025
@bors
Copy link
Contributor

bors commented Jan 2, 2025

⌛ Testing commit 4bf85c2 with merge ab3924b...

@bors
Copy link
Contributor

bors commented Jan 3, 2025

☀️ Test successful - checks-actions
Approved by: the8472
Pushing ab3924b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 3, 2025
@bors bors merged commit ab3924b into rust-lang:master Jan 3, 2025
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Jan 3, 2025
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ab3924b): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.7% [0.7%, 0.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 1.4%, secondary 2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [0.7%, 2.1%] 3
Regressions ❌
(secondary)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [0.7%, 2.1%] 3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.3%, secondary 0.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.3% [0.1%, 0.7%] 15
Regressions ❌
(secondary)
0.7% [0.5%, 0.7%] 38
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.1%, 0.7%] 15

Bootstrap: 763.571s -> 763.678s (0.01%)
Artifact size: 325.58 MiB -> 325.50 MiB (-0.02%)

@Zoxc Zoxc deleted the atomic-panic-msg branch January 3, 2025 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.

Scoped thread panic messages unexpectedly interleaving in stderr output