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

assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")) panic at build on i386, i586, FreeBSD, or many target_os=none x86 targets. #1999

Open
girgen opened this issue Mar 18, 2024 · 18 comments

Comments

@girgen
Copy link

girgen commented Mar 18, 2024

Hi!

I get this error when building an app using ring-0.17.8

error[E0080]: evaluation of constant value failed
 --> /wrkdirs/usr/ports/www/sqlpage/work/SQLpage-0.20.0/cargo-crates/ring-0.17.8/src/cpu/[intel.rs:28](http://intel.rs:28/):9
  |
28 |         assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2"));
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'assertion failed: cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")', /wrkdirs/usr/ports/www/sqlpage/work/SQLpage-0.20.0/cargo-crates/ring-0.17.8/src/cpu/[intel.rs:28](http://intel.rs:28/):9
  |
  = note: this error originates in the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0080`.
error: could not compile `ring` (lib) due to 1 previous error

I tried to find info using the three issue links in the code, but I don't have enough experience with rust, to be honest, I'm just packaging an application. It seems to me that the problem is with the assert call itself, so perhaps this is actually a bug?

Removing the check will make the code build.

Do you need more information, just specify what is needed. Build log attached.

sqlpage-0.20.0.log

@girgen girgen changed the title Fails to build on i386 for FreeBSD assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")), ails to build on i386 for FreeBSD Mar 18, 2024
@girgen girgen changed the title assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")), ails to build on i386 for FreeBSD assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")) panic at build on i386 for FreeBSD Mar 18, 2024
@briansmith
Copy link
Owner

What are the steps to reproduce? cargo build --target=????

@girgen
Copy link
Author

girgen commented Mar 22, 2024

See the attached log above ☝️

Looks like

--target i686-unknown-freebsd

@briansmith
Copy link
Owner

What do you get when you do this?

$ rustc --print=cfg --target i686-unknown-freebsd | grep sse
debug_assertions
target_feature="sse"
target_feature="sse2"

For me, rustc indicates that sse and sse2 are there.

I see in your build log that there are warnings that CARGO_CFG_TARGET_FEATURE not set. THat might be a clue.

Make sure you're not disabling SSE/SSSE2 in .cargo/config.toml or otherwise.

@girgen
Copy link
Author

girgen commented Mar 26, 2024

% rustc --print=cfg --target i686-unknown-freebsd | grep sse
debug_assertions

Seems like rust is configured for pentiumpro (no SSEx) in the FreeBSD ports tree?

See https://github.com/freebsd/freebsd-ports/blob/main/lang/rust/files/patch-compiler_rustc__target_src_spec_targets_i686__unknown__freebsd.rs

@briansmith briansmith changed the title assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")) panic at build on i386 for FreeBSD assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")) panic at build on i386 (for FreeBSD), i586, or many target_os=none x86 targets. May 7, 2024
@briansmith briansmith changed the title assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")) panic at build on i386 (for FreeBSD), i586, or many target_os=none x86 targets. assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")) panic at build on i386, i586, FreeBSD, or many target_os=none x86 targets. May 7, 2024
@briansmith
Copy link
Owner

At https://www.freebsd.org/releases/14.0R/hardware/#proc-i386, it is written:

2.3. i386 Architecture Support
FreeBSD maintains support for i386 (x86) as a Tier 2 architecture. It is not recommended for new installations.

Support for this architecture will be removed in FreeBSD 15.0.

@dch
Copy link

dch commented Jun 5, 2024

FreeBSD 15.0 hasn't been released yet, so this is in the future.
We still have FreeBSD 13.3-RELEASE and 14.1-RELEASE active, which
will run for several more years yet.

For me, this blocks e.g. lang/gleam on i386, which admittedly is neither
a popular port, nor a very common platform yet.

Do we amend the i386 target on FreeBSD, for rust? @MikaelUrankar I know you're familiar with the FreeBSD rust porting, what do you suggest here?

Perhaps we should discuss this in bugzilla?

@briansmith
Copy link
Owner

Note that you can build ring with RUSTFLAGS="-C target-feature=+sse2" (or whatever) to get it to work on those targets. Note that it might have built before I added these assertions, but if you tried to run it it would execute SSE2 instructions without checking at runtime whether the system actually supported SSE2 instructions.

If you actually want ring to support x86 systems that don't have SSE2, then we'd need somebody to submit a PR that adds the runtime checking for SSE2 to every assembly language function called. They are easy to find; look for target_arch="x86" in the source code. To see how to add the check for SSE2, look at how the check for SSSE3 is done by searching for SSSE3.

@he32
Copy link

he32 commented Jun 10, 2024

I'll just note that I'm observing the same problem for the i586-unknown-netbsd target. It also does not support the SSE or SSE2 instructions, and targets Pentium and above, for maximal backward compatibility:

$ rustc --print=cfg --target i586-unknown-netbsd | grep sse
debug_assertions
$ rustc --print=cfg | grep sse
debug_assertions
$ uname -rps
NetBSD 9.3 i386
$ rustc --version
rustc 1.78.0 (9b00956e5 2024-04-29) (built from a source tarball)
$

This is seen for ring-0.17.8:

error[E0080]: evaluation of constant value failed
  --> /usr/pkgsrc/net/routinator/work/vendor/ring-0.17.8/src/cpu/intel.rs:28:9
   |
28 |         assert!(cfg!(target_feature = "sse") && cfg!(target_feature = "sse2"));
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'assertion failed: cfg!(target_feature = "sse") && cfg!(target_feature = "sse2")', /usr/pkgsrc/net/routinator/work/vendor/ring-0.17.8/src/cpu/intel.rs:28:9

@briansmith
Copy link
Owner

Yes, the also applies to that target.

@guenhter
Copy link

@briansmith I'd like to help out supporting the i586 target. After looking into the code, I have no clue what exactly I should do. Also looking at SSSE3 didn't really help.
Do I only have to adapt the rust code or also the .pl stuff? Would it be possible to show me one example what I have to do, so that I can do it then on all other places where target_arch="x86" is set.

@Fabian-Gruenbichler
Copy link

this also affects Debian's i386 arch, since that has neither SSE nor SSE2. not building ring anymore there would remove a huge chunk of Rust packages in turn. is anybody already working on such a conditionalizing patch?

@plugwash
Copy link

I've written a patch that forces a generic no-asm build on x86 targets without sse2 and uploaded it to Debian.

https://salsa.debian.org/rust-team/debcargo-conf/-/blob/d586098f2597e5b5a1dcbf87af5268b3b84206e1/src/ring/debian/patches/use-generic-implementation-on-non-sse2-x86.patch

Obviously this is not optimal, runtime detection would be better but it stops this being a rc bug for us.

@briansmith
Copy link
Owner

I am open to addressing this issue but I don't intend to work on it myself any time soon except to review a PR that fixes it and which is tested in GitHub Actions CI in some way. I suggest we use i586-unknown-linux-gnu as the target to test in GitHub Actions. The PR should make the SSE/SSE2 detection work like other feature detection currently works and which matches the current (at the time of reviewing the PR) coding conventions in ring.

@guenhter
Copy link

@briansmith I'm completly open to make the PR. It would just be very helpful if you could show me a single location where action is required and what needs to be done. The rest can be done by me.

@briansmith
Copy link
Owner

briansmith commented Sep 28, 2024

@briansmith I'm completly open to make the PR. It would just be very helpful if you could show me a single location where action is required and what needs to be done. The rest can be done by me.

  • In cpu::intel, you need to do the analog of this to define a new SSE2, for target_arch = "x86" only. The mask value needs to be looked up in the intel reference:
#[allow(dead_code)]
pub(crate) const SSE41: Feature = Feature {
    word: 1,
    mask: 1 << 19,
};
  • Look at all the files named x86-*.pl or *-x86.pl. Each of these files declares some functions like &function_begin("bn_mul_mont");. If the function name doesn't start with an underscore then it is extern. For all such extern functions in these files (only), rename the function to add a suffix "_sse2", e.g. bn_mul_mont_sse2.

  • Change build.rs to add these *_sse2 functions' names to the list in SYMBOLS_TO_PREFIX.

  • For each prefixed_extern! { fn foo() } in the Rust code that mentions target_arch = "x86", you'll need to augment/replace it with #[cfg(target_arch = "x86")] prefixed_extern! { fn foo_sse2() } and you'll need to define a new implementation of foo that looks something like this:

#[cfg(target_arch = "x86")]
extern "C" unsafe fn foo(...) { 
         if cfg!(target_feature = "sse2") || cpu::intel::SSE2.available(cpu::features())) {
             unsafe { foo_sse2(....) };
         } else {
             <Call the fallback implementation that is used on targets that don't support assembly language.>
         }
}

I suggest you do this with bn_mul_mont first. You'll find it declared in montgomery.rs and defined (for 32-bit x86) in x86-mont.pl. In the case of bn_mul_mont you'll probably want to start by splitting the Rust implementation of bn_mul_mont into two parts, one being bn_mul_mont_fallback that contains the actual fallback implementation, and bn_mul_mont that is a thin wrapper around bn_mul_mont_fallback. Note that bn_mul_mont is special because it is also called by C code so it needs the prefixed_export! hack mentioned in montgomery.rs.

In some cases, the assembly code already depends on some other feature, like AES-NI. In that case, we can change OPENSSL_cpuid_setup so that no features are detected if SSE and SSE2 aren't detected; this might already be the case. For these cases where some other feature is being detected, you won't need to do any work. It is only when the fallback implementation on 32-bit x86 is an assembly function that you need to take the above steps.

You can get a sense for how much work this is by going through the code looking for "prefixed_extern!" that has a target_arch = "x86" in its cfg!; for each one, check the caller(s) to see if they are already doing feature detection for x86; if so, you don't need to touch that one. Otherwise, if the function is the fallback implementation, you need to take the above steps.

@guenhter
Copy link

Thx for this comprehensive guide. I'll do my best to implement that.
From a timeline perspective I hope to be able to start with this in the next weeks.

@briansmith
Copy link
Owner

Great!

In terms of testing, look at how qemu is used in CI to test on older CPUs. Then you can run cargo test under QEMU with a very old CPU model specified to see that it is working.

@jackpot51
Copy link
Contributor

jackpot51 commented Oct 18, 2024

@briansmith would it help if a PR was made with the patch that debian is using? It looks okay to me.

Here is the comparison, it needs to be rebased but will look the same: main...redox-os:ring:redox-0.17.8

jackpot51 added a commit to redox-os/ring that referenced this issue Oct 18, 2024
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

8 participants