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

emitter: column width defaults to 140 #73719

Merged
merged 1 commit into from
Jun 26, 2020

Conversation

davidtwco
Copy link
Member

Fixes #72509.

This PR modifies the column width computation in the emitter when termize::dimensions returns None so that it uses the default value of 140 (which is used in UI testing currently) instead of usize::MAX which just ends up causing overflows in later computations.

I also tried changing the computations which used column_width with their saturating equivalent, but the output appeared the same - so I decided to go with this approach because I feel like it's less likely to accidentally re-introduce an ICE like this in future (e.g. adding a non-saturating operation on column_width in future).

I haven't added a test because I couldn't come up with a MCVE. I stumbled upon this running rustc-perf with the piston-image benchmark (running in tmux; it only happened with stage two builds only; and only when running through Cargo, not rustc directly with the same flags). In addition, given the nature of the issue, I don't know that we could write a UI test for this. Open to suggestions here though.

r? @estebank

This commit modifies the column width computation in the emitter when
`termize::dimensions` returns `None` so that it uses the default value
of 140 (which is used in UI testing currently) instead of `usize::MAX`
which just ends up causing overflows in later computations. This is hard
to test but appears to produce the same output as using saturating
functions instead.

Signed-off-by: David Wood <david@davidtw.co>
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2020
@estebank
Copy link
Contributor

I'm slightly concerned that this is going to start affect cargo output, whereas now it was supposed to act as if this feature didn't exist (until I could get around to land the feature support in cargo). That being said, I think this is a reasonable change. It might break cargo's tests though.

You couldn't make a test for this because on tests it uses 140, and you can only override it with a specific number, not with an unbounded value.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 25, 2020

📌 Commit 11a3584 has been approved by estebank

@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 Jun 25, 2020
@estebank
Copy link
Contributor

@davidtwco would you have the time to drive rust-lang/cargo#7315 to completion?

@davidtwco
Copy link
Member Author

@davidtwco would you have the time to drive rust-lang/cargo#7315 to completion?

I should do, I'll familiarize myself with it.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 25, 2020
…n-width, r=estebank

emitter: column width defaults to 140

Fixes rust-lang#72509.

This PR modifies the column width computation in the emitter when `termize::dimensions` returns `None` so that it uses the default value of 140 (which is used in UI testing currently) instead of `usize::MAX` which just ends up causing overflows in later computations.

I also tried changing the computations which used `column_width` with their saturating equivalent, but the output appeared the same - so I decided to go with this approach because I feel like it's less likely to accidentally re-introduce an ICE like this in future (e.g. adding a non-saturating operation on `column_width` in future).

I haven't added a test because I couldn't come up with a MCVE. I stumbled upon this running rustc-perf with the `piston-image` benchmark (running in tmux; it only happened with stage two builds only; and only when running through Cargo, not rustc directly with the same flags). In addition, given the nature of the issue, I don't know that we *could* write a UI test for this. Open to suggestions here though.

r? @estebank
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 26, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72620 (Omit DW_AT_linkage_name when it is the same as DW_AT_name)
 - rust-lang#72967 (Don't move cursor in search box when using arrows to navigate results)
 - rust-lang#73102 (proc_macro: Stop flattening groups with dummy spans)
 - rust-lang#73297 (Support configurable deny-warnings for all in-tree crates.)
 - rust-lang#73507 (Cleanup MinGW LLVM linkage workaround)
 - rust-lang#73588 (Fix handling of reserved registers for ARM inline asm)
 - rust-lang#73597 (Record span of `const` kw in GenericParamKind)
 - rust-lang#73629 (Make AssocOp Copy)
 - rust-lang#73681 (Update Chalk to 0.14)
 - rust-lang#73707 (Fix links in `SliceIndex` documentation)
 - rust-lang#73719 (emitter: column width defaults to 140)
 - rust-lang#73729 (disable collectionbenches for android)
 - rust-lang#73748 (Add code block to code in documentation of `List::rebase_onto`)

Failed merges:

r? @ghost
@bors bors merged commit 97ccd97 into rust-lang:master Jun 26, 2020
@davidtwco davidtwco deleted the issue-72509-emitter-column-width branch July 2, 2020 12:28
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 14, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72620 (Omit DW_AT_linkage_name when it is the same as DW_AT_name)
 - rust-lang#72967 (Don't move cursor in search box when using arrows to navigate results)
 - rust-lang#73102 (proc_macro: Stop flattening groups with dummy spans)
 - rust-lang#73297 (Support configurable deny-warnings for all in-tree crates.)
 - rust-lang#73507 (Cleanup MinGW LLVM linkage workaround)
 - rust-lang#73588 (Fix handling of reserved registers for ARM inline asm)
 - rust-lang#73597 (Record span of `const` kw in GenericParamKind)
 - rust-lang#73629 (Make AssocOp Copy)
 - rust-lang#73681 (Update Chalk to 0.14)
 - rust-lang#73707 (Fix links in `SliceIndex` documentation)
 - rust-lang#73719 (emitter: column width defaults to 140)
 - rust-lang#73729 (disable collectionbenches for android)
 - rust-lang#73748 (Add code block to code in documentation of `List::rebase_onto`)

Failed merges:

r? @ghost
@cuviper cuviper added this to the 1.46 milestone May 2, 2024
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: attempt to add with overflow', src/librustc_errors/emitter.rs:128:29
5 participants