search

supranational / blst

Multilingual BLS12-381 signature library
Apache License 2.0
463 stars 178 forks source link

Rust bindings not recompiled on target CPU change #207

Closed nazar-pc closed 6 months ago

nazar-pc commented 6 months ago

Compiling rust library with -C target-cpu=skylake followed by -C target-cpu=x86-64-v2 results in blst still compiled with ADX instructions and crashing on user's machine.

I believe build.rs needs to account for this and recompile C sources in case target or individual CPU features change.

dot-asm commented 6 months ago

I can't confirm this. At least if I do env RUSTFLAGS="-C target-cpu=skylake" cargo build followed by env RUSTFLAGS="-C target-cpu=x86-64-v2" cargo build in the bindings/rust directory, the resulting libblst.rlib doesn't have mulx symbols. And this is obviously because cargo appears to redo everything upon RUSTFLAGS change, including build script invocation, which does pay attention to the target features...

nazar-pc commented 6 months ago

Hm, interesting. Thanks for testing so quickly!

I'm actually using some more advanced features of Rust Nightly like this:

cargo -Zgitoxide -Zgit build --locked -Z build-std --target x86_64-unknown-linux-gnu --profile production

Nothing is obviously wrong with any of those flags, but https://github.com/subspace/space-acres/pull/165 did certainly fix the problem, so at least with that combination of flags it should be reproducible.

dot-asm commented 6 months ago

One can wonder if it's a cargo bug. Upon RUSTFLAGS change it always verbose-logs "Dirty blst ... : the rustflags changed" and "Compiling blst", but if explicit --target option is passed, it doesn't execute the build script... Again, no --target option, build script is executed, pass --target, it's not...

nazar-pc commented 6 months ago

Very interesting bug indeed, I'll report it to Cargo after some more experiments. Will close this issue for now, thanks!

dot-asm commented 6 months ago

At some point users at cc-rs complained that cargo stopped rebuilding things after C source code modifications. And it turned out to be contingent on the fact that cc-rs started issuing cargo:rerun-if-env-changed directives by default, which set aside the default [non-Rust] file modification detection mechanism. Yes, environment modification detection affected the file modification detection. With this in mind it's a lesser stretch to imagine that the directives in question could affect RUSTFLAGS modification detection, because it's an environment variable. Well, this doesn't justify the dependency on --target, but maybe it's an intentional behaviour. Or both are bugs...

nazar-pc commented 6 months ago

Reduced to -Z build-std causing issues, reported to Cargo: https://github.com/rust-lang/cargo/issues/13597