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

array impls <= 32 1.39 regression #65522

Closed
Mark-Simulacrum opened this issue Oct 17, 2019 · 12 comments
Closed

array impls <= 32 1.39 regression #65522

Mark-Simulacrum opened this issue Oct 17, 2019 · 12 comments
Assignees
Labels
E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum Mark-Simulacrum added this to the 1.39 milestone Oct 17, 2019
@Mark-Simulacrum Mark-Simulacrum added E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 17, 2019
@scottmcm
Copy link
Member

scottmcm commented Oct 18, 2019

cc PR #62435 No, turns out that was in 1.38

[INFO] [stderr]   the trait `std::array::LengthAtMost32` is not implemented for `[i8; 128]`
[INFO] [stderr]   |
[INFO] [stderr]   = note: required because of the requirements on the impl of `std::fmt::Debug` for `[i8; 128]`
[INFO] [stderr]   = note: required because of the requirements on the impl of `std::fmt::Debug` for `&[i8; 128]`
[INFO] [stderr]   = note: required for the cast to the object type `dyn std::fmt::Debug`

I guess this used to unsize to a slice?

@bluss
Copy link
Member

bluss commented Oct 19, 2019

Formatting the first it points to this:

error[E0277]: arrays only have std trait implementations for lengths 0..=32
   --> src/lib.rs:362:5
    |
362 |     pub data: [::std::os::raw::c_char; 128usize],
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::array::LengthAtMost32` is not implemented for `[i8; 128]`
    |
    = note: required because of the requirements on the impl of `std::fmt::Debug` for `[i8; 128]`
    = note: required because of the requirements on the impl of `std::fmt::Debug` for `&[i8; 128]`
    = note: required for the cast to the object type `dyn std::fmt::Debug`

in

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct duk_thread_state {
    pub data: [::std::os::raw::c_char; 128usize],
}

And it fails to compile in Rust 1.37, 1.38 as well, so it's not a regression. Since this is generated code, it could be that it's a change in what code is being generated, but it seems unlikely?

@pnkfelix
Copy link
Member

pnkfelix commented Oct 24, 2019

triage: P-high (at least to get to bottom of what actual bug is here)

@pnkfelix pnkfelix added the P-high High priority label Oct 24, 2019
@LukasKalbertodt
Copy link
Member

I tried to investigate a bit. The problematic code:

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct duk_thread_state {
    pub data: [::std::os::raw::c_char; 128usize],
}

Does not compile in any compiler version after >1.0 (well, I only tested 1.30 -- 1.38, beta, nightly and 1.0, but I'm pretty sure that code was never accepted by the compiler).

This piece of code is generated by bindgen from a C library. The C library is part of the crate (i.e. it's not downloaded by the build script) and thus doesn't change. It can't be the reason for this breakage. So maybe bindgen changed its code generation? duktape_ffi_raw uses bindgen = "0.49.0", but there does not seem to be any relevant changes in the versions 0.49.0 to 0.49.3.

However, the other regression, libhydrogen-sys also uses bindgen to generate code. So it looks highly unlikely this is a spurious regression. libhydrogen-sys uses bindgen = "0" by the way (something that I luckily haven't seen before).

Maybe we could just ping someone from bindgen and ask them if something relevant changed?

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Oct 28, 2019

@LukasKalbertodt This is probably the same as the other bindgen-related regressions. According to @pcpthm in #65525:

Likely due to #64710 exposing bindgen bug rust-lang/rust-bindgen#1627. Updating bindgen fixes the issue.

I've not verified this, however.

@LukasKalbertodt
Copy link
Member

I noticed something else: the documentation on docs.rs about the problematic struct of duktape_ffi_raw shows that Debug is not implemented for the struct. So the bindgen code generation did indeed change. At some point the Debug derive as not added, and thus the crate compiled fine. The regression is caused by derive(Debug) being added for some reason.

@ecstatic-morse I can't connect the bug you linked to what we are seeing here... but maybe it is indeed connected.

@tesuji
Copy link
Contributor

tesuji commented Oct 29, 2019

I'm pretty sure that code was never accepted by the compiler

You're right. Tested on godbolt I came to the same conclusion: https://rust.godbolt.org/z/uS3ST2

@pcpthm
Copy link
Contributor

pcpthm commented Oct 29, 2019

Indeed it seems like another case of bindgen bug.
The bindgen bug affects the code-path of determining whether #[derive(Trait)] can be applied or not.
By default, bindgen applies #[derive(Debug)] whenever possible. The error matches the situation of when #[derive(Debug)] is applied to an underiveable struct, due to the bug.
Updating bindgen dependency should resolve the issue (though I'm not able to test this).

@pnkfelix
Copy link
Member

assigning self to attempt to verify that this is fixed by upgrading bindgen

@pnkfelix pnkfelix self-assigned this Oct 31, 2019
@nikomatsakis
Copy link
Contributor

Testing:

  • duktape_ffi_raw is fixed by upgrading to bindgen 0.51

@pnkfelix
Copy link
Member

pnkfelix commented Nov 4, 2019

Testing:

  • docteurklein looks like it is fixed by upgrading to bindgen 0.51.1 (or at least, the rust compilation problem goes away, and the errors I get building on my test bed environments the ones I see on stable.)

@pnkfelix
Copy link
Member

pnkfelix commented Nov 4, 2019

closing as expected fallout from catching latent bug (now fixed) in bindgen crate.

@pnkfelix pnkfelix closed this as completed Nov 4, 2019
@Centril Centril removed this from the 1.39 milestone Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants