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

wrong code regression: HashSet initialization #42918

Closed
Dushistov opened this issue Jun 26, 2017 · 11 comments
Closed

wrong code regression: HashSet initialization #42918

Dushistov opened this issue Jun 26, 2017 · 11 comments
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. O-android Operating system: Android O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Dushistov
Copy link
Contributor

Dushistov commented Jun 26, 2017

Idea of bug is simple:

use std::collections::HashSet;

#[derive(PartialEq, Debug, Hash, Eq, Clone)]
enum SentenceType {
    None,
    AAM,
    //...many
    RMC,
    GGA,
    //..many
}

fn f(s: HashSet<SentenceType>) {
    println!("s: {:?}", s);
}

fn main() {
    f([SentenceType::RMC, SentenceType::GGA]
          .iter()
          .cloned()
          .collect());
}

On stable today compiler rustc 1.18.0 (03fc9d622 2017-06-06) it print as expected {RMC, GGA}, but if use beta/nightly it prints all variants from SentenceType.

Note: that the code above is just sketch, to realy reproduce problem you need android/arm cpu (may be works with other/arm, but I not able to check right now),
and two crates, in attachment crate that reproduce problem,
to run it you may use such commands:

adb shell mkdir /data/sample
cargo build   --target=arm-linux-androideabi --release
adb push target/arm-linux-androideabi/release/hashset_bug /data/sample/
adb shell 'RUST_LOG=debug /data/sample/hashset_bug'

With stable it produces such output:

DEBUG:hashset_bug: Hello, world!
DEBUG:nmea: create_for_navigation-beta: arg {RMC, GGA}
DEBUG:nmea: create_for_navigation: our {RMC, GGA}

with beta/nightly it prodcues such output:

DEBUG:hashset_bug: Hello, world!
DEBUG:nmea: create_for_navigation-beta: arg {MTW, GTD, DSI, ZDL, RMB, MWD, DSR, TUT, ZDA, RSA, HSC, CUR, RSD, OLN, XDR, RTE, ABK, DBK, SFI, HDG, BWR, GRS, DTM, LRF, MWV, ACA, TLB, HTD, SSD, GSV, RMA, MSK, APB, MSS, GLC, HDT, WCV, VHW, ACK, MLA, ASD, GXA, RMC, APA, ZFO, LR3, GNS, AAM, ZTG, GGA, FSI, BWW, XTR, VPW, AIR, VWR, XTE, BWC, LRI, BEC, TTM, LR2, LCD, ROT, VDO, GBS, GMP, VDR, GSA, RPM, ALM, VSD, GST, TRF, LR1, DCN, VDM, DPT, TXT, VLW, HMR, DSE, None, VBW, BOD, GLL, VTG, DBS, WNC, TLL, ACS, DSC, ALR, STN, DBT, HMS, HDM}
DEBUG:nmea: create_for_navigation: our {MTW, GTD, DSI, ZDL, RMB, MWD, DSR, TUT, ZDA, RSA, HSC, CUR, RSD, OLN, XDR, RTE, ABK, DBK, SFI, HDG, BWR, GRS, DTM, LRF, MWV, ACA, TLB, HTD, SSD, GSV, RMA, MSK, APB, MSS, GLC, HDT, WCV, VHW, ACK, MLA, ASD, GXA, RMC, APA, ZFO, LR3, GNS, AAM, ZTG, GGA, FSI, BWW, XTR, VPW, AIR, VWR, XTE, BWC, LRI, BEC, TTM, LR2, LCD, ROT, VDO, GBS, GMP, VDR, GSA, RPM, ALM, VSD, GST, TRF, LR1, DCN, VDM, DPT, TXT, VLW, HMR, DSE, None, VBW, BOD, GLL, VTG, DBS, WNC, TLL, ACS, DSC, ALR, STN, DBT, HMS, HDM}

hashset_bug.zip

@Mark-Simulacrum Mark-Simulacrum added O-android Operating system: Android O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed O-android Operating system: Android O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state labels Jun 26, 2017
@nagisa nagisa added A-codegen Area: Code generation I-wrong regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jun 27, 2017
@nikomatsakis
Copy link
Contributor

Nominating. cc @rust-lang/compiler -- anybody have any ideas what's going on with this ARM codegen regression?

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jul 7, 2017

So we've updated to LLVM 4.0.1 since then (#42930) -- I suspect that may well have fixed this problem. Perhaps @Dushistov could check on a recent nightly? In case the problem isn't quite fixed, cc @arielb1

@Dushistov
Copy link
Contributor Author

Dushistov commented Jul 7, 2017

@Mark-Simulacrum

So we've updated to LLVM 4.0.1 since then (#42930) -- I suspect that may well have fixed this problem.

Yes, with rustc 1.20.0-nightly (696412de7 2017-07-06) all ok.

At now only 1.19.0-beta.2 (a175ee509 2017-06-15) FAILED.

@Mark-Simulacrum
Copy link
Member

I believe that the fix is backported, just we haven't released a new beta yet.

@brson
Copy link
Contributor

brson commented Jul 7, 2017

It looks to me like the llvm patch has not been backported yet. I will do so today.

@Mark-Simulacrum
Copy link
Member

@brson #42948 appears to be the backport?

@brson
Copy link
Contributor

brson commented Jul 7, 2017

@Mark-Simulacrum hm so it is. it is tagged incorrectly.

@brson
Copy link
Contributor

brson commented Jul 13, 2017

beta.3 is out with the LLVM backport. Can anyone confirm a fix?

@Dushistov
Copy link
Contributor Author

@brson rustc 1.19.0-beta.3 (4b6f4f353 2017-07-07) fix issue. Any ideas how to reduce test case, or it is small enough for regression test?

@brson brson added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Jul 13, 2017
@brson
Copy link
Contributor

brson commented Jul 13, 2017

Thanks for confirming @Dushistov. I don't offhand know how to reduce the test case though. Sorry!

@Mark-Simulacrum Mark-Simulacrum removed I-nominated regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jul 13, 2017
@Mark-Simulacrum Mark-Simulacrum added C-bug Category: This is a bug. and removed I-wrong labels Jul 28, 2017
@Dushistov
Copy link
Contributor Author

Dushistov commented Aug 25, 2017

I was able to reduce the test to this code:

use std::collections::HashSet;

#[derive(PartialEq, Debug, Hash, Eq, Clone)]
enum MyEnum {
    E0,

    E1,

    E2,
    E3,
    E4,

    E5,
    E6,
    E7,
}


fn main() {
    let s: HashSet<_> = [MyEnum::E4, MyEnum::E1].iter().cloned().collect();

    println!("Expect only E1+E4 got {:?}", s);
}

it prints

Expect only E1+E4 got {E4, E0, E6, E3, E7, E1, E2, E5}

on buggy compier

Dushistov added a commit to Dushistov/rust that referenced this issue Aug 26, 2017
This is test for rust-lang#42918.
To reproduce bug you need machine with arm cpu and compile with optimization.
I tried with rustc 1.19.0-nightly (3d5b8c6 2017-06-09),
if compile test with -C opt-level=3 for target=arm-linux-androideabi
and run on "Qualcomm MSM 8974 arm cpu" then assert fails,
if compile and run with -C opt-level=2 it gives segmentation fault.
So I add `compile-flags: -O`.
With rustc 1.19.0 (0ade339 2017-07-17) all works fine.
Closes rust-lang#42918
bors added a commit that referenced this issue Aug 26, 2017
Add test for wrong code generation for HashSet creation on arm cpu

This is test for #42918.
To reproduce bug you need machine with arm cpu and compile with optimization.
I tried with rustc 1.19.0-nightly (3d5b8c6 2017-06-09),
if compile test with -C opt-level=3 for target=arm-linux-androideabi
and run on "Qualcomm MSM 8974 arm cpu" then assert fails,
if compile and run with -C opt-level=2 it gives segmentation fault.
So I add `compile-flags: -O`.
With rustc 1.19.0 (0ade339 2017-07-17) all works fine.
Closes #42918
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. O-android Operating system: Android O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state 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

5 participants