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

Compilation Issue on rust 1.41.0 #68

Closed
dbcfd opened this issue Feb 12, 2020 · 10 comments · Fixed by #70
Closed

Compilation Issue on rust 1.41.0 #68

dbcfd opened this issue Feb 12, 2020 · 10 comments · Fixed by #70
Labels

Comments

@dbcfd
Copy link

dbcfd commented Feb 12, 2020

error[E0428]: the name `imp` is defined multiple times
   --> .cargo/registry/src/github.com-1ecc6299db9ec823/memchr-2.3.1/src/lib.rs:148:5
    |
139 |     fn imp(n1: u8, haystack: &[u8]) -> Option<usize> {
    |     ------------------------------------------------ previous definition of the value `imp` here
...
148 |     fn imp(n1: u8, haystack: &[u8]) -> Option<usize> {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `imp` redefined here
    |
    = note: `imp` must be defined only once in the value namespace of this block
@dbcfd
Copy link
Author

dbcfd commented Feb 12, 2020

System info

info: using existing install for 'stable-x86_64-apple-darwin'
info: default toolchain set to 'stable-x86_64-apple-darwin'

  stable-x86_64-apple-darwin unchanged - rustc 1.41.0 (5e1a79984 2020-01-27)

@dbcfd
Copy link
Author

dbcfd commented Feb 12, 2020

This also happens on beta and nightly.

@BurntSushi
Copy link
Owner

I'ved yanked 2.3.1 from crates.io until I have time to properly investigate this. cc @ldesgoui

(It was almost certainly caused by #67.)

@BurntSushi BurntSushi added the bug label Feb 12, 2020
@BurntSushi
Copy link
Owner

I'm particular, I would like to know why CI didn't catch this.

@CryZe
Copy link

CryZe commented Feb 12, 2020

https://github.com/BurntSushi/rust-memchr/pull/67/files#diff-b4aea3e418ccdb71239b96952d9cddb6R145

This should be not(miri)

Though the problem seems to be in more places.

@ldesgoui
Copy link
Contributor

My bad! I'll be home in 10 to fix, not(all(a, b, c)) should be all(not(all(a, b)), not(c))

@BurntSushi
Copy link
Owner

No rush. It is yanked so there should be anyone impacted by this now. But yeah, thank you. I also need to figure out how to make sure this case is tested in CI.

@kali
Copy link

kali commented Feb 12, 2020

If it can help, I see it from a github action CI here https://github.com/snipsco/tract/runs/441345982?check_suite_focus=true

@ldesgoui
Copy link
Contributor

ldesgoui commented Feb 12, 2020

The compilation error is caused by the logic detecting when the C version should be used, which is memchr_libc AND (NOT (x86_64 AND memchr_runtime_simd)) AND (NOT miri) which translates to
cfg(all( memchr_libc, not(all(target_arch = "x86_64", memchr_runtime_simd)), not(miri) )
Here's the truth table comparing what we have/what we want:

x64 simd miri !(x64 && simd && miri) !(x64 && simd) && !miri
f f f t t
f f t t f wrong
f t f t t
f t t t f wrong
t f f t t
t f t t f wrong
t t f t f wrong
t t t f f

4 wrong cases, the one hit by users was most likely features = "libc", x64 = t, simd = t, miri = f

@ldesgoui
Copy link
Contributor

This case is not tested in there:
https://github.com/BurntSushi/rust-memchr/blob/2.3.1/.github/workflows/ci.yml#L100-L122
Incoming PR with fix and according CI test

BurntSushi pushed a commit that referenced this issue Feb 13, 2020
This fixes the cfg knobs, which were producing a compilation error
when memchr had its libc feature enabled without also disabling
automatic SIMD acceleration. This particular combination was
accidentally not tested in CI, so this ended up breaking builds that
had memchr's libc feature enabled.

This also adds tests that will specifically be run under miri.

Fixes #68, Fixes #69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants