Closed Bug 1433747 Opened 2 years ago Closed 2 years ago

--enable-rust-simd fails to build for -C target-cpu with AVX2

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- unaffected

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

(Keywords: regression)

Attachments

(2 files)

$ c++ -v
FreeBSD clang version 6.0.0 (branches/release_60 323338) (based on LLVM 6.0.0)
Target: x86_64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /usr/bin

$ rustc -vV
rustc 1.23.0
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-freebsd
release: 1.23.0
LLVM version: 4.0

$ echo "ac_add_options --enable-rust-simd" >>.mozconfig
$ echo 'export RUSTFLAGS="-C target-cpu=haswell"' >>.mozconfig
$ ./mach build
[...]
   Compiling simd v0.2.1
error[E0442]: intrinsic return value has wrong type: found vector with length 4, expected length 32
  --> third_party/rust/simd/src/x86/avx2.rs:45:5
   |
45 |     fn x86_mm256_sad_epu8(x: u8x32, y: u8x32) -> u64x4;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
could you file a issue to https://github.com/rust-lang-nursery/simd ?
Flags: needinfo?(jbeich)
Priority: -- → P3
(In reply to Makoto Kato [:m_kato] from comment #2)
> could you file a issue to https://github.com/rust-lang-nursery/simd ?

If you follow the link in See Also field it seems upstream won't fix it.
Flags: needinfo?(jbeich)
(In reply to Jan Beich from comment #3)
> (In reply to Makoto Kato [:m_kato] from comment #2)
> > could you file a issue to https://github.com/rust-lang-nursery/simd ?
> 
> If you follow the link in See Also field it seems upstream won't fix it.

rustc still uses llvm-5.x, so after upgrading to 6.x, it might be changed.
(In reply to Makoto Kato [:m_kato] from comment #4)
> rustc still uses llvm-5.x,

Did you use a time machine? Even Nightly is still at llvm-4.x.

$ rustc -vV
rustc 1.25.0-nightly (def3269a7 2018-01-30)
binary: rustc
commit-hash: def3269a71be2e737cad27418a3dad9f5bd6cd32
commit-date: 2018-01-30
host: x86_64-unknown-freebsd
release: 1.25.0-nightly
LLVM version: 4.0

https://github.com/rust-lang/llvm/blob/bc344d5bc23c/CMakeLists.txt#L23
https://github.com/rust-lang/rust/issues/43370

(In reply to Makoto Kato [:m_kato] from comment #4)
> so after upgrading to 6.x, it might be changed.

Rust 1.24 (to be released on 2018-02-15) isn't affected, so the fix is probably to bump minimum required version.
https://searchfox.org/mozilla-central/rev/c56f656febb1/build/moz.configure/rust.configure#70
Depends on: 1430927
Attachment #8946105 - Attachment description: workaround → Workaround for Rust < 1.24
How often Gentoo or Arch users build Firefox with custom RUSTFLAGS e.g., for -C target-cpu=native?
Flags: needinfo?(jan-steffens)
Flags: needinfo?(axs)
Arch doesn't build with custom RUSTFLAGS.

(And please don't use that old email address. Where did you find it?)
Flags: needinfo?(jan-steffens)
For Firefox < 60 the issue manifests with Rust >= 1.24.

$ c++ -v
FreeBSD clang version 4.0.0 (tags/RELEASE_400/final 297347) (based on LLVM 4.0.0)
Target: x86_64-unknown-freebsd11.1
Thread model: posix
InstalledDir: /usr/bin

$ rustc -vV
rustc 1.24.0-beta.8 (ed9751a90 2018-01-23)
binary: rustc
commit-hash: ed9751a903b483769d3d418818b331c867f0417f
commit-date: 2018-01-23
host: x86_64-unknown-freebsd
release: 1.24.0-beta.8
LLVM version: 4.0

$ hg up beta
$ echo "ac_add_options --enable-rust-simd" >>.mozconfig
$ echo 'export RUSTFLAGS="-C target-cpu=haswell"' >>.mozconfig
$ ./mach build
[...]
error[E0442]: intrinsic return value has wrong type: found vector with length 32, expected length 4
  --> third_party/rust/simd/src/x86/avx2.rs:45:5
   |
45 |     fn x86_mm256_sad_epu8(x: u8x32, y: u8x32) -> u8x32;
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
error: Could not compile `simd`.

(In reply to Jan Alexander Steffens [:heftig] from comment #7)
> (And please don't use that old email address. Where did you find it?)

Email auto-completion. If you still have access to that mail or can login maybe merge into current one e.g., bug 1021105.
Attachment #8946105 - Attachment description: Workaround for Rust < 1.24 → Firefox >= 60 workaround for Rust < 1.24
Please don’t ship with --enable-rust-simd

This flag subverts Rust’s stability mechanism to use unstable language features despite using a stable compiler release. Which is probably ok for local experimentation, but perhaps we should better communicate that it can and will break.

I worry that if we consider such breakage a bug at all, there will be pressure for unfinished Rust features to be considered de-facto stable. Setting that precedent could "break" the Rust ecosystem for everyone. (This won’t happen overnight, but let’s not take any step in that direction.)
I've added it to the Arch builds as it's apparently used for the official builds (via mozconfig.rust via mozconfig.common).
(In reply to Simon Sapin (:SimonSapin) from comment #10)
> Please don’t ship with --enable-rust-simd
> 
> This flag subverts Rust’s stability mechanism to use unstable language
> features despite using a stable compiler release. Which is probably ok for
> local experimentation, but perhaps we should better communicate that it can
> and will break.
> 
> I worry that if we consider such breakage a bug at all, there will be
> pressure for unfinished Rust features to be considered de-facto stable.
> Setting that precedent could "break" the Rust ecosystem for everyone. (This
> won’t happen overnight, but let’s not take any step in that direction.)

Well, that's not what was advised in bug 1261841 comment 177, and i know i've been shipping with --enable-rust-simd since then because of that comment.
(In reply to Simon Sapin (:SimonSapin) from comment #10)
> Please don’t ship with --enable-rust-simd

I think we shouldn't ask distros to ship a less fast Firefox than Mozilla ships. And Mozilla should keep shipping with --enable-rust-simd, because Firefox needs to be maximally fast now and not some time in the future.
When I first learned about this flag I misread the config and thought we didn’t ship with it. So never mind my "please don’t" comment.

Shipping with unstable features is a choice we can make but it is not "free". Breakage such as in bug is exactly what should be expected. I think we should at least communicate more clearly about that. "Enable simd" by itself sounds like nothing more than "go faster". For example, we could have a --disable-rust-versions-compatibility flag, document it (do we have centralized documentation of build flags?) as "Using any Rust version other than the one used in Mozilla builds may not work", and have --enable-rust-simd require it.
(In reply to Simon Sapin (:SimonSapin) from comment #14)
> --disable-rust-versions-compatibility

How would this help downstream? Reminds me of bug 1371159.

> "Using any Rust version other than the one used in Mozilla builds may not work", 

Why not have a Tier2 job on TreeHerder that uses Rust Nightly? Maybe skip inbound/autoland to save CPU cycles.

> and have --enable-rust-simd require it.

--enable-stylo is less likely to survive every Rust and LLVM release during ESR60 lifetime than --enable-rust-simd. Here's how bad it already looks:

Stylo from Firefox 58 doesn't build with Rust 1.25.
Stylo from Firefox 57 doesn't build with Rust 1.23.
Stylo from Firefox 56 doesn't build with Rust 1.21.
(In reply to Jan Beich from comment #6)
> How often Gentoo or Arch users build Firefox with custom RUSTFLAGS e.g., for
> -C target-cpu=native?

Gentoo by default doesn't do anything with RUSTFLAGS, but end-users can certainly set that variable in their system profiles (just like CFLAGS) and it will propagate through to the build environment for mozilla.  Gentoo's policy is to honour the values set in system profiles and only strip flags when its absolutely necessary, so unless setting RUSTFLAGS is considered to be entirely unsupported and causing breakage even under unrelated circumstances (ie like setting -C target-cpu=native), I would prefer not to strip or unset that variable in our build environment.
Flags: needinfo?(axs)
(In reply to Jan Beich from comment #15)
> Stylo from Firefox 58 doesn't build with Rust 1.25.
> Stylo from Firefox 57 doesn't build with Rust 1.23.
> Stylo from Firefox 56 doesn't build with Rust 1.21.

That’s definitely a failure on our part. Thank you for the CC on bug 1434619. Are there other bugs for Stylo/Rust compat? Though if they’re all about #![deny(warnings)] that’s an easy fix. We’ve already discussed removing that line before, I’m not sure why we haven’t yet.

However I don’t see how this is relevant to Firefox abusing Rust’s mechanism for compiling itself, in order to use features that are explicitly marked as susceptible to breaking changes.
(In reply to Jan Beich from comment #15)

> Why not have a Tier2 job on TreeHerder that uses Rust Nightly? Maybe skip
> inbound/autoland to save CPU cycles.

This depends on `-C target_cpu` right? I don't think we set that for Rust code in Mozilla's builds, so a regular run wouldn't have caught it.

Anyway, setting up periodic builds with beta and nightly rust is bug 1337955.

It's unfortunate we can't ifdef it to support both rustc versions. Maybe the best fix is to jump the epoch and require 1.24 for Firefox 60.
> It's unfortunate we can't ifdef it to support both rustc versions.

It’s not super easy but it’s possible. Have a Cargo build script that has uses https://crates.io/crates/rustc_version to parse the output of `rustc --version`, and based on that conditionally tell Cargo to pass `--cfg something`. Then in the library code, use #[cfg(something)] and #[cfg(not(something))] attributes to conditionally compile.

By the way, I see that the patches attached modify files under third_party/rust/ which are normally considered read-only. I think the next `./mach vendor rust` run would overwrite those changes?
(In reply to Simon Sapin (:SimonSapin) from comment #10)
> Please don’t ship with --enable-rust-simd
> 
> This flag subverts Rust’s stability mechanism to use unstable language
> features despite using a stable compiler release. Which is probably ok for
> local experimentation, but perhaps we should better communicate that it can
> and will break.
> 
> I worry that if we consider such breakage a bug at all, there will be
> pressure for unfinished Rust features to be considered de-facto stable.
> Setting that precedent could "break" the Rust ecosystem for everyone. (This
> won’t happen overnight, but let’s not take any step in that direction.)

It is used for encodibng-rs.  Could I turn off this flag as default?
Flags: needinfo?(hsivonen)
(In reply to Makoto Kato [:m_kato] from comment #20)
> (In reply to Simon Sapin (:SimonSapin) from comment #10)
> > Please don’t ship with --enable-rust-simd
...
> It is used for encodibng-rs.  Could I turn off this flag as default?

Please don't change the flag in Mozilla's builds. That would regress performance.

The flag is already off by default so that developers don't accidentally introduce dependencies on nightly-only features. (Depending on nightly-only SIMD is not accidental.) The flag is intentionally turned on in Mozilla's CI. Also, distros are right to turn it on to match Mozilla's release configuration.

(In reply to Jan Beich from comment #0)
> $ echo 'export RUSTFLAGS="-C target-cpu=haswell"' >>.mozconfig

Does setting a different target-cpu than what Mozilla builds with yield concrete benefits?

> error[E0442]: intrinsic return value has wrong type: found vector with
> length 4, expected length 32
>   --> third_party/rust/simd/src/x86/avx2.rs:45:5
>    |
> 45 |     fn x86_mm256_sad_epu8(x: u8x32, y: u8x32) -> u64x4;
>    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

AFAICT, nothing calls this function, so the simplest fix would be to delete the line instead of changing it, but see below.

(In reply to Simon Sapin (:SimonSapin) from comment #19)
> By the way, I see that the patches attached modify files under
> third_party/rust/ which are normally considered read-only. I think the next
> `./mach vendor rust` run would overwrite those changes?

Indeed. This makes me reluctant to make any changes here unless there's a very good reason for FreeBSD to override target-cpu in the first place. We could downgrade the simd crate to the previous crates.io release, since the changes in upstream don't affect the configuration that Mozilla builds with anyway. (I now regret upgrading the crate just for the sake of minimizing version deviation from what's current at crates.io.) But if we downgraded the crate now, FreeBSD would have a compilation problem when they upgrade rustc. As long as FreeBSD is the only one overriding target-cpu, we could choose the simd crate version based on FreeBSD's rustc, but that doesn't scale if another distro starts tweaking target-cpu.

Is there a good reason why FreeBSD can't refrain from overriding target-cpu until its rustc has caught up with these changes?
Flags: needinfo?(hsivonen)
(In reply to Henri Sivonen (:hsivonen) from comment #21)
> The flag is already off by default so that developers don't accidentally
> introduce dependencies on nightly-only features. (Depending on nightly-only
> SIMD is not accidental.)

For clarity: "nightly" in those sentences refers to nightly Rust--not to nightly Firefox.
Firefox 60 now requires rust 1.24 or later. Jan, please feel free to land the appropriate update.
Assignee: nobody → jbeich
- Nothing to land for Firefox 60 as Rust < 1.24 isn't supported anymore
- Firefox < 60 cannot be fixed without risking to break older Rust versions (e.g., as used by Mozilla automation) but bumping minimum required version for Firefox < 59 may require bug 1434619 fix as well
- attachment 8947037 [details] [diff] [review] can still be used when downstream upgrades Rust during Firefox 58/59 lifetime, assuming someone actually notices the bustage
- attachment 8946105 [details] [diff] [review] can still be used if dowstream fails to upgrade system Rust after Firefox 60 release, assuming Rust 1.22-1.24 features will remain unused for ESR60 lifetime. For example, DragonFly (Tier3) is currently stuck on Rust 1.21.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
(In reply to Jan Beich from comment #24)
> assuming Rust 1.22-1.24 features will remain unused for ESR60...

Ignore this. I forgot 1.22 and 1.23 bumps happened a while ago. Building ESR60 with Rust < 1.23 may not be trivial.
You need to log in before you can comment on or make changes to this bug.