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

i686-unknown-linux-musl gives builds sizeof(int64_t) == 4 on 64-bit hosts #436

Closed
alecmocatta opened this issue Aug 25, 2019 · 5 comments
Closed

Comments

@alecmocatta
Copy link

alecmocatta commented Aug 25, 2019

See for example this assertion raised by the miniz-sys crate on a stock 64-bit ubuntu machine:

$ cargo test --target i686-unknown-linux-musl
   Compiling miniz-sys v0.1.12 (/home/alec/flate2-rs/miniz-sys)
error: failed to run custom build command for `miniz-sys v0.1.12 (/home/alec/flate2-rs/miniz-sys)`

Caused by:
  process didn't exit successfully: `/home/alec/flate2-rs/target/debug/build/miniz-sys-db79608bd7405c9f/build-script-build` (exit code: 1)
--- stdout
TARGET = Some("i686-unknown-linux-musl")
OPT_LEVEL = Some("0")
HOST = Some("x86_64-unknown-linux-gnu")
CC_i686-unknown-linux-musl = None
CC_i686_unknown_linux_musl = None
TARGET_CC = None
CC = None
CROSS_COMPILE = None
CFLAGS_i686-unknown-linux-musl = None
CFLAGS_i686_unknown_linux_musl = None
TARGET_CFLAGS = None
CFLAGS = None
CRATE_CC_NO_DEFAULTS = None
DEBUG = Some("true")
CARGO_CFG_TARGET_FEATURE = Some("crt-static,fxsr,mmx,sse,sse2")
running: "musl-gcc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-g" "-fno-omit-frame-pointer" "-m32" "-static" "-march=i686" "-Wl,-melf_i386" "-fvisibility=hidden" "-DMINIZ_NO_STDIO" "-DMINIZ_NO_ARCHIVE_APIS" "-DMINIZ_NO_ARCHIVE_WRITING_APIS" "-DMINIZ_NO_TIME" "-DMINIZ_NO_ZLIB_COMPATIBLE_NAMES" "-o" "/home/alec/flate2-rs/target/i686-unknown-linux-musl/debug/build/miniz-sys-0ae776d4c6f5a15f/out/miniz.o" "-c" "miniz.c"
cargo:warning=miniz.c:31:23: error: size of array ‘mz_validate_uint64’ is negative
cargo:warning= typedef unsigned char mz_validate_uint64[sizeof(mz_uint64) == 8 ? 1 : -1];
cargo:warning=                       ^~~~~~~~~~~~~~~~~~
exit code: 1

--- stderr


error occurred: Command "musl-gcc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-g" "-fno-omit-frame-pointer" "-m32" "-static" "-march=i686" "-Wl,-melf_i386" "-fvisibility=hidden" "-DMINIZ_NO_STDIO" "-DMINIZ_NO_ARCHIVE_APIS" "-DMINIZ_NO_ARCHIVE_WRITING_APIS" "-DMINIZ_NO_TIME" "-DMINIZ_NO_ZLIB_COMPATIBLE_NAMES" "-o" "/home/alec/flate2-rs/target/i686-unknown-linux-musl/debug/build/miniz-sys-0ae776d4c6f5a15f/out/miniz.o" "-c" "miniz.c" with args "musl-gcc" did not execute successfully (status code exit code: 1).

I don't know if or where this should be fixed? musl-gcc encodes the target triple (in my case x86_64-linux-musl) so an i386 version of musl-gcc needs to be installed and used.

Perhaps asserting on musl-gcc -v 2>&1 | grep Target would be useful? I was getting silent corruption of data with the zstd crate; a build error would be much preferred.

@alexcrichton
Copy link
Member

Hm it looks like we're already passing all the right flags, so is there much we can do here other than providing a more first-class error?

@alecmocatta
Copy link
Author

I agree that not much can be done besides raising an error. I suggested above checking the output of musl-gcc -v, but I realise that isn't totally robust. The most robust way I think would be a test compile, effectively:

echo '#include <stdint.h>\ntypedef unsigned char validate_uint64[sizeof(uint64_t) == 8 ? 1 : -1];' > test.c
musl-gcc <args> -S -o /dev/null test.c

I think most crates using cc (how many assert the size of uint64_t??), in most environments, will currently silently miscompile when cross-compiling to i686-unknown-linux-musl so personally I think checking this would be valuable.

Personally I'd be in favour of adding it as a general sanity check for all targets here, implemented in a similar way to is_flag_supported.

What are your thoughts? Is it worth doing, and if so only for 32bit musl, for all musl, or for all targets?

@alexcrichton
Copy link
Member

Ok sounds like if we were to do this it'd be a sanity check compile, but even that seems like it's sort of a lot to do with this crate. I definitely agree it's something real bad to run into, but I'm not sure how this can be handled robustly across platforms and targets, this is sort of just a "c compilers are hard" place I think

@alecmocatta
Copy link
Author

even that seems like it's sort of a lot to do with this crate

I feel similarly, but for me the upsides outweight the downsides.

As a "worst case" example, rust-secp256k1 miscompiles (with various warnings like right shift count >= width of type) with borked cryptographic key generation. This may well result in factorable public keys.

A sanity test compile takes 5-30ms on my machines. I can't think of a scenario where this crate doing such a test compile would provide a false negative. Where are you thinking it wouldn't be robust? And would it be okay to provide an opt-out env flag for these scenarios?

@alexcrichton
Copy link
Member

I personally would prefer to not maintain this in this crate, it runs the risk of being a pretty sprawling piece of logic to what's an already sprawling crate. It'd be great if an extension on crates.io could be published which can be combined with cc to add sanity checks so crates could opt-in to it, but I think I would personally prefer to not maintain such support in this crate itself.

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

No branches or pull requests

2 participants