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

VecDeque from Vec fails with ZST #78532

Closed
alecmocatta opened this issue Oct 29, 2020 · 10 comments · Fixed by #80003
Closed

VecDeque from Vec fails with ZST #78532

alecmocatta opened this issue Oct 29, 2020 · 10 comments · Fixed by #80003
Labels
A-collections Area: `std::collection` A-zst Area: Zero-sized types (ZST). C-bug Category: This is a bug. P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@alecmocatta
Copy link
Contributor

This code panics under Miri, and also when I compile to WASM, though not when running normally.

use std::collections::VecDeque;

fn main() {
    let _: VecDeque<()> = vec![()].into();
}

Playground

thread 'main' panicked at 'attempt to add with overflow', /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/collections/vec_deque.rs:3236:36

The overflow is here:

let cap = cmp::max(buf.capacity() + 1, MINIMUM_CAPACITY + 1).next_power_of_two();

@alecmocatta alecmocatta added the C-bug Category: This is a bug. label Oct 29, 2020
@jonas-schievink jonas-schievink added A-collections Area: `std::collection` T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 29, 2020
@camelid camelid added the A-zst Area: Zero-sized types (ZST). label Oct 29, 2020
@camelid
Copy link
Member

camelid commented Oct 29, 2020

I can reproduce when using Miri, but it works fine for me with WASM:

$ cargo +stable build --target=wasm32-unknown-unknown
   Compiling rust-issue-78532 v0.1.0 (/Users/user/rust-issue-78532)
    Finished dev [unoptimized + debuginfo] target(s) in 0.17s

(Same with nightly toolchain.)

@lcnr
Copy link
Contributor

lcnr commented Oct 29, 2020

use std::collections::VecDeque;

fn main() {
    let v = vec![(); 100];
    let queue = VecDeque::from(v);
    println!("{:?}", queue);
}

panics with

thread 'main' panicked at 'capacity overflow', library/alloc/src/raw_vec.rs:542:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

it always fails here because len is greater than MINIMUM_CAPACITY so cap - len underflows.

@lcnr lcnr added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 29, 2020
@camelid
Copy link
Member

camelid commented Oct 29, 2020

Why is this specific to ZSTs?

@lcnr
Copy link
Contributor

lcnr commented Oct 29, 2020

Vec does not allocate for ZST and starts out with capacity usize::MAX which overflows when converting to a deque which does not expect this

@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 29, 2020
@camelid
Copy link
Member

camelid commented Oct 29, 2020

Assigning P-high and removing I-prioritize as discussed in the prioritization working group.

@RalfJung
Copy link
Member

RalfJung commented Nov 1, 2020

This code panics under Miri, and also when I compile to WASM, though not when running normally.

Currently on playground it seems to work fine both under Miri and regular rustc? Indeed something would be very wrong if it only panicked on Miri...

@lcnr's version panics both with rustc and Miri. Maybe the example in the OP should be updated.

@alecmocatta
Copy link
Contributor Author

Currently on playground it seems to work fine both under Miri and regular rustc? Indeed something would be very wrong if it only panicked on Miri...

Yes, though at time of posting it did fail under miri (and only under miri) on the playground.

I can reproduce when using Miri, but it works fine for me with WASM

Sorry if I was unclear; I meant that I got the same panic when running the built WASM.

@RalfJung
Copy link
Member

RalfJung commented Nov 2, 2020

Yes, though at time of posting it did fail under miri (and only under miri) on the playground.

My guess is that something in libstd changed to fix the panic for your case (but not for @lcnr's), and when you tested this, Miri was broken in the latest toolchain so playground had an older version of Miri. When this happens again, please note down the exact versions of every component involved, so we can make sure this is comparing Miri and rustc for the same toolchain, not two different versions of Rust.

@tmiasko
Copy link
Contributor

tmiasko commented Nov 3, 2020

The originally reported panic is an overflow check and one of those that is not inherited cross-crate. To observe it the standard library must be built with overflow checks. It's still an issue right now.

@RalfJung
Copy link
Member

RalfJung commented Nov 3, 2020

Ah, so this stopped to fail in Miri when we recently stopped building libstd with debug assertions.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 14, 2020
…on-panic, r=dtolnay

Fix overflow when converting ZST Vec to VecDeque

```rust
let v = vec![(); 100];
let queue = VecDeque::from(v);
println!("{:?}", queue);
```
This code will currently panic with a capacity overflow.
This PR resolves this issue and makes the code run fine.

Resolves rust-lang#78532
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 15, 2020
…on-panic, r=dtolnay

Fix overflow when converting ZST Vec to VecDeque

```rust
let v = vec![(); 100];
let queue = VecDeque::from(v);
println!("{:?}", queue);
```
This code will currently panic with a capacity overflow.
This PR resolves this issue and makes the code run fine.

Resolves rust-lang#78532
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 15, 2020
…on-panic, r=dtolnay

Fix overflow when converting ZST Vec to VecDeque

```rust
let v = vec![(); 100];
let queue = VecDeque::from(v);
println!("{:?}", queue);
```
This code will currently panic with a capacity overflow.
This PR resolves this issue and makes the code run fine.

Resolves rust-lang#78532
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 15, 2020
…on-panic, r=dtolnay

Fix overflow when converting ZST Vec to VecDeque

```rust
let v = vec![(); 100];
let queue = VecDeque::from(v);
println!("{:?}", queue);
```
This code will currently panic with a capacity overflow.
This PR resolves this issue and makes the code run fine.

Resolves rust-lang#78532
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 16, 2020
…on-panic, r=dtolnay

Fix overflow when converting ZST Vec to VecDeque

```rust
let v = vec![(); 100];
let queue = VecDeque::from(v);
println!("{:?}", queue);
```
This code will currently panic with a capacity overflow.
This PR resolves this issue and makes the code run fine.

Resolves rust-lang#78532
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 16, 2020
…on-panic, r=dtolnay

Fix overflow when converting ZST Vec to VecDeque

```rust
let v = vec![(); 100];
let queue = VecDeque::from(v);
println!("{:?}", queue);
```
This code will currently panic with a capacity overflow.
This PR resolves this issue and makes the code run fine.

Resolves rust-lang#78532
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 16, 2020
…on-panic, r=dtolnay

Fix overflow when converting ZST Vec to VecDeque

```rust
let v = vec![(); 100];
let queue = VecDeque::from(v);
println!("{:?}", queue);
```
This code will currently panic with a capacity overflow.
This PR resolves this issue and makes the code run fine.

Resolves rust-lang#78532
@bors bors closed this as completed in 2e9ed6f Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` A-zst Area: Zero-sized types (ZST). C-bug Category: This is a bug. P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants