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

Don't call `FieldPlacement::count` when count is too large #57042

Merged
merged 1 commit into from Jan 12, 2019

Conversation

Projects
None yet
6 participants
@pnkfelix
Copy link
Member

pnkfelix commented Dec 21, 2018

Sidestep ICE in FieldPlacement::count by not calling it when count will not fit in host's usize.

(I briefly played with trying to fix this by changing FieldPlacement::count to return a u64. However, based on how FieldPlacement is used, it seems like this would be a largely pointless pursuit... I'm open to counter-arguments, however.)

Fix #57038

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 21, 2018

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 21, 2018

Cc @eddyb

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Dec 21, 2018

Note that this PR doesn't actually make it so that our test suite runs cleanly for a host=i686 adn target=x86_64.

Namely, I observe this failure on huge-enum.rs now atop this PR.

failures:

---- [ui] ui/huge-enum.rs stdout ----
diff of stderr:

-       error: the type `std::option::Option<[u32; N]>` is too big for the current architecture
+       error: the type `[u32; 35184372088831]` is too big for the current architecture
2
3       error: aborting due to previous error
4

So ... is this an improvement over the status quo (and we just should revise the test suite a touch, maybe)? Or should we consider a broader change so that we get closer behavior in these various contexts...?

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

michaelwoerister commented Jan 9, 2019

Let's merge it for now. It's definitely an improvement.

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 9, 2019

📌 Commit 0eacf2c has been approved by michaelwoerister

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

⌛️ Testing commit 0eacf2c with merge 5edef53...

bors added a commit that referenced this pull request Jan 10, 2019

Auto merge of #57042 - pnkfelix:issue-57038-sidestep-ice-in-fieldplac…
…ement-count, r=michaelwoerister

Don't call `FieldPlacement::count` when count is too large

Sidestep ICE in `FieldPlacement::count` by not calling it when count will not fit in host's usize.

(I briefly played with trying to fix this by changing `FieldPlacement::count` to return a `u64`. However, based on how `FieldPlacement` is used, it seems like this would be a largely pointless pursuit... I'm open to counter-arguments, however.)

Fix #57038
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 10, 2019

@bors retry
AppVeyor... what's wrong with you today?

Centril added a commit to Centril/rust that referenced this pull request Jan 10, 2019

Rollup merge of rust-lang#57042 - pnkfelix:issue-57038-sidestep-ice-i…
…n-fieldplacement-count, r=michaelwoerister

Don't call `FieldPlacement::count` when count is too large

Sidestep ICE in `FieldPlacement::count` by not calling it when count will not fit in host's usize.

(I briefly played with trying to fix this by changing `FieldPlacement::count` to return a `u64`. However, based on how `FieldPlacement` is used, it seems like this would be a largely pointless pursuit... I'm open to counter-arguments, however.)

Fix rust-lang#57038

bors added a commit that referenced this pull request Jan 11, 2019

Auto merge of #57503 - Centril:rollup, r=Centril
Rollup of 19 pull requests

Successful merges:

 - #56425 (Redo the docs for Vec::set_len)
 - #56906 (Issue #56905)
 - #57042 (Don't call `FieldPlacement::count` when count is too large)
 - #57192 (Change std::error::Error trait documentation to talk about `source` instead of `cause`)
 - #57296 (Fixed the link to the ? operator)
 - #57353 (Optimise floating point `is_finite` (2x) and `is_infinite` (1.6x).)
 - #57368 (Use CMAKE_{C,CXX}_COMPILER_LAUNCHER for ccache)
 - #57400 (Rustdoc: update Source Serif Pro and replace Heuristica italic)
 - #57412 (Improve the wording)
 - #57417 (rustdoc: use text-based doctest parsing if a macro is wrapping main)
 - #57433 (Add link destination for `read-ownership`)
 - #57434 (Remove `CrateNum::Invalid`.)
 - #57441 (Supporting backtrace for x86_64-fortanix-unknown-sgx.)
 - #57450 (actually take a slice in this example)
 - #57454 (Some cleanups for core::fmt)
 - #57459 (Reference tracking issue for inherent associated types in diagnostic)
 - #57463 (docs: Fix some 'second-edition' links)
 - #57466 (Remove outdated comment)
 - #57493 (use structured suggestion when casting a reference)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Jan 11, 2019

Auto merge of #57503 - Centril:rollup, r=Centril
Rollup of 19 pull requests

Successful merges:

 - #56425 (Redo the docs for Vec::set_len)
 - #56906 (Issue #56905)
 - #57042 (Don't call `FieldPlacement::count` when count is too large)
 - #57192 (Change std::error::Error trait documentation to talk about `source` instead of `cause`)
 - #57296 (Fixed the link to the ? operator)
 - #57353 (Optimise floating point `is_finite` (2x) and `is_infinite` (1.6x).)
 - #57368 (Use CMAKE_{C,CXX}_COMPILER_LAUNCHER for ccache)
 - #57400 (Rustdoc: update Source Serif Pro and replace Heuristica italic)
 - #57412 (Improve the wording)
 - #57417 (rustdoc: use text-based doctest parsing if a macro is wrapping main)
 - #57433 (Add link destination for `read-ownership`)
 - #57434 (Remove `CrateNum::Invalid`.)
 - #57441 (Supporting backtrace for x86_64-fortanix-unknown-sgx.)
 - #57450 (actually take a slice in this example)
 - #57454 (Some cleanups for core::fmt)
 - #57459 (Reference tracking issue for inherent associated types in diagnostic)
 - #57463 (docs: Fix some 'second-edition' links)
 - #57466 (Remove outdated comment)
 - #57493 (use structured suggestion when casting a reference)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jan 12, 2019

Rollup merge of rust-lang#57042 - pnkfelix:issue-57038-sidestep-ice-i…
…n-fieldplacement-count, r=michaelwoerister

Don't call `FieldPlacement::count` when count is too large

Sidestep ICE in `FieldPlacement::count` by not calling it when count will not fit in host's usize.

(I briefly played with trying to fix this by changing `FieldPlacement::count` to return a `u64`. However, based on how `FieldPlacement` is used, it seems like this would be a largely pointless pursuit... I'm open to counter-arguments, however.)

Fix rust-lang#57038

bors added a commit that referenced this pull request Jan 12, 2019

Auto merge of #57542 - Centril:rollup, r=Centril
Rollup of 26 pull requests

Successful merges:

 - #56425 (Redo the docs for Vec::set_len)
 - #56906 (Issue #56905)
 - #57042 (Don't call `FieldPlacement::count` when count is too large)
 - #57175 (Stabilize `let` bindings and destructuring in constants and const fn)
 - #57192 (Change std::error::Error trait documentation to talk about `source` instead of `cause`)
 - #57296 (Fixed the link to the ? operator)
 - #57368 (Use CMAKE_{C,CXX}_COMPILER_LAUNCHER for ccache)
 - #57400 (Rustdoc: update Source Serif Pro and replace Heuristica italic)
 - #57417 (rustdoc: use text-based doctest parsing if a macro is wrapping main)
 - #57433 (Add link destination for `read-ownership`)
 - #57434 (Remove `CrateNum::Invalid`.)
 - #57441 (Supporting backtrace for x86_64-fortanix-unknown-sgx.)
 - #57450 (actually take a slice in this example)
 - #57459 (Reference tracking issue for inherent associated types in diagnostic)
 - #57463 (docs: Fix some 'second-edition' links)
 - #57466 (Remove outdated comment)
 - #57493 (use structured suggestion when casting a reference)
 - #57498 (make note of one more normalization that Paths do)
 - #57499 (note that FromStr does not work for borrowed types)
 - #57505 (Remove submodule step from README)
 - #57510 (Add a profiles section to the manifest)
 - #57511 (Fix undefined behavior)
 - #57519 (Correct RELEASES.md for 1.32.0)
 - #57522 (don't unwrap unexpected tokens in `format!`)
 - #57530 (Fixing a typographical error.)
 - #57535 (Stabilise irrefutable if-let and while-let patterns)

Failed merges:

r? @ghost

@bors bors merged commit 0eacf2c into rust-lang:master Jan 12, 2019

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment