Miri: non-deterministic floating point operations in `foreign_items` #143906

LorrensP-2158466 · 2025-07-13T20:52:02Z

Part of rust-lang/miri/#3555, this pr does the foreign_items work.

Some things have changed since #138062 and #142514. I moved the "helpers" used for creating fixed outputs and clamping operations to their defined ranges to math.rs. These are now also extended to handle the floating-point operations in foreign_items. Tests in miri/tests/float.rs were changed/added.

Failing tests in std were extracted, run under miri with -Zmiri-many-seeds=0..1000 and changed accordingly. Double checked with -Zmiri-many-seeds.

I noticed that the C standard doesn't specify the output ranges for all of its mathematical operations; it just specifies them as:

Returns
The sinh functions return sinh x.

So I used Wolfram|Alpha.

rustbot · 2025-07-13T20:52:07Z

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-13T20:52:09Z

The Miri subtree was changed

cc @rust-lang/miri

LorrensP-2158466 · 2025-07-13T20:52:48Z

There are some holes in the behaviour of some operations, because I did not know how I could efficiently handle them, I'll mark them and add some explanation.

Also,
r? @RalfJung

src/tools/miri/src/helpers.rs

src/tools/miri/tests/pass/float.rs

src/tools/miri/src/helpers.rs

oli-obk · 2025-07-14T08:02:47Z

Please create a PR for the libstd changes on their own so a libs reviewer can review it. Once that is done and synced, you can open a PR against the miri repo with the miri changes.

RalfJung · 2025-07-14T10:43:28Z

In a previous PR we had both in one PR with two reviewers since we needed to go back and forth a bit. We could do that again? I think it worked well.

LorrensP-2158466 · 2025-07-14T10:57:32Z

It's the same for me, I kept the commits for Miri and stdlib separate, so if I have to split it up, it's pretty straightforward.

RalfJung · 2025-07-14T11:59:03Z

🤷 I'm also fine either way, it'll just be a bunch of work to port the commits to the Miri repo.

I noticed that the C standard doesn't specify the output ranges for all of its mathematical operations; it just specifies them as:

Functions where C does not give an output range shouldn't get their value clamped, I would say.

src/tools/miri/src/helpers.rs

RalfJung · 2025-07-14T12:00:38Z

I moved the "helpers" used for creating fixed outputs and clamping operations to their defined ranges to helpers.rs. These are now also extended to handle the floating-point operations in foreign_items. Tests in miri/tests/float.rs were changed/added.

Please don't, that file is already too big.^^

If the helpers are only needed in one file, keep them there.
Otherwise, add them in src/tools/miri/src/math.rs.

src/tools/miri/src/math.rs

LorrensP-2158466 · 2025-07-14T20:17:29Z

Reopening.

LorrensP-2158466 · 2025-07-14T20:35:25Z

Functions where C does not give an output range shouldn't get their value clamped, I would say.

You're right, but this has some consequences. For example, the sin operation doesn't have an explicitly defined fixed output range, yet we guarantee and test this in Miri: rust-lang/miri#4207.

The C standard for the return values of sin is:

Returns
The sin functions return sin x.

The range of sine x is [-1, 1], but is not explicitly defined.

But asin has the following description:

Returns
The asin functions return arcsin x in the interval [−π/2, +π/2] radians.

I think we have 2 approaches:

We say this is implementation-defined and revert all the clamps that are not explicitly defined
We implicitly read the mathematically defined output ranges of those operations.

tgross35 · 2025-07-14T21:19:33Z

If we run into libm implementations that can't even guarantee to return a value within the math function's output domain, tbh I would consider that downright broken and worthy of a bug report, or strong reason to just always use our libm on that platform. I'd be a bit hesitant to encourage users to think about this via Miri unless we think there is a chance this is actually a problem in practice (do we know of any platforms where this has been a problem?).

If it's mostly a concern about following the specification to the letter, I think it might be worth a defect report to see if the C committee is willing to codeify the output domain.

RalfJung · 2025-07-14T21:20:02Z

I think we have 2 approaches:

I would suggest a third:

We do this kind of clamping wherever C mandates it, plus wherever it is easy and/or useful. So, fine to do it for sin and cos, but not for the gamma function ;)

RalfJung · 2025-07-14T21:21:23Z

But asin has the following description:

The arcsin function likely states this not as a means of guaranteeing precision, but because the inverse sine of x is under-defined -- infinitely many different inputs map to x (for x in [-1, 1]). By giving the interval, the answer becomes uniquely defined.

matthiaskrgr · 2025-07-22T08:25:44Z

@bors r-

#144286 (comment)
https://triage.rust-lang.org/gha-logs/rust-lang/rust/46447815197

LorrensP-2158466 · 2025-07-22T12:25:51Z

Can't seem to reproduce it on my machine. Might as well set everything to 1e-5 for 32 and 1e-10 for f64.

@tgross35, @RalfJung, what do you think?

RalfJung · 2025-07-23T09:49:19Z

That sounds reasonable to me (but you have some f32 tests that currently use 1e-4).

LorrensP-2158466 · 2025-07-23T10:40:12Z

I'll set them all to 1e-4 then.

LorrensP-2158466 · 2025-07-23T14:01:23Z

I'll edit the last commit with --keep-base.

LorrensP-2158466 · 2025-07-23T14:19:28Z

Hi, @tgross35, would you like to look at the last commit? There are a few more stdlib changes now. Changes from f32::EPSILON to 1e-* where part of this pr.

RalfJung · 2025-07-23T14:21:21Z

This is not setting all the f32 ones to 1e-4 (take a look at the diff). Not that I mind.^^

LorrensP-2158466 · 2025-07-23T14:21:32Z

library/std/src/num/f64.rs

@@ -754,7 +754,7 @@ impl f64 {
    /// // asin(sin(pi/2))
    /// let abs_difference = (f.sin().asin() - std::f64::consts::FRAC_PI_2).abs();
    ///
-    /// assert!(abs_difference < 1e-7);
+    /// assert!(abs_difference < 1e-5);


This failed because of the double operations, but it's weird this one is so unprecise when running under miri, while others like cos and tan are not.

I tried 1000 seeds and 1e-7 always worked. Why did you reduce to 1e-5?

(1e-8 failed quickly.)

it's weird this one is so unprecise when running under miri, while others like cos and tan are not.

asin seems to be quite sensitive around input 1, which is what we are using here.
And yeah, looking at WA, its derivative explodes towards 1: https://www.wolframalpha.com/input?i2d=true&i=D%5Basin%5C%2840%29x%5C%2841%29%2Cx%5D.

In contrast, we are evaluating acos at around 0.7, where its derivative is much lower.

Maybe we should change the test to an area where asin is more stable -- like pi/4, which the other 2 tests are already using?

I commented this on the wrong test, but you're last 2 replies apply to that test as well.

Okay, but you did change this one to 1e-5... why?

I tried 1000 seeds and 1e-7 always worked

This is not a good indicator; I also did this check, but it still failed in CI. Thus, I changed it based on the assumption that this one would fail similarly in CI.

If we hit a 1/1000 event in CI the first try, that is worth further investigation as it indicates something rather unlikely.

I'm sorry, I was a little fast in my reply. The test that failed in CI was asinh, but that one was also battle-tested with 1000 seeds. That CI failure just prompted me to change this test as well.

LorrensP-2158466 · 2025-07-23T14:22:48Z

This is not setting all the f32 ones to 1e-4 (take a look at the diff). Not that I mind.^^

Yeah, just noticed. :)

@rustbot ready

library/std/src/num/f32.rs

LorrensP-2158466 · 2025-07-24T13:40:22Z

library/std/src/num/f32.rs

@@ -1067,7 +1067,7 @@ impl f32 {
    ///
    /// let abs_difference = (f - x).abs();
    ///
-    /// assert!(abs_difference <= 1e-7);
+    /// assert!(abs_difference <= 1e-4);


It's this one that failed, not the one I commented here https://github.com/rust-lang/rust/pull/143906/files#r2225770166

Maybe we should change the test to an area where asin is more stable -- like pi/4, which the other 2 tests are already using?

Agreed.

This one might be amplifying an underlying error introduced in hypot or ln_1p due to how it is implemented.

But 1000 tests should still be enough for all practical purposes... strange.

LorrensP-2158466 · 2025-07-25T16:11:32Z

Testing with Zmiri-many-seeds=0..1000 is not enough, as one test (f32::asinh) still failed in CI.

It's Friday, and I don't have much to do, so I'm going to extract every changed f32/f64 test that was changed and run it with Miri with a much larger range. (In isolation, it is manageable.)

If some tests still fail under CI I propose we just change them so they can never fail with Miri and track them elsewhere?

LorrensP-2158466 · 2025-07-25T18:11:44Z

With Zmiri-many-seeds=0..10000 testing f32, the limits are:

abs_sub = 1e-06
acos = 1e-06
acosh = 1e-05
asin = 1e-01
asinh = 1e-06 
atan = 1e-06
atan2 = 1e-05
atanh = 1e-04
cbrt = 1e-06
cos = 1e-01
cosh = 1e-06
erfc = 1e-06
exp = 1e-06
exp2 = 1e-05
exp_m1 = 1e-14
gamma = 1e-04
hypot = 1e-05
ln = 1e-06
ln_1p = 1e-14
log = 1e-06
log10 = 1e-06
log2 = 1e-06
powf = 1e-05
powi = 1e-05
sin = 1e-06
sinh = 1e-06
tan = 1e-06
tanh = 1e-06

RalfJung · 2025-07-25T18:19:15Z

Can you put them in this PR, except for the cases where the limit before the PR also already works (i.e., minimize the diff)?

LorrensP-2158466 · 2025-07-31T19:03:32Z

Sorry for the delay, stuck with some other work and wanted to be sure of the current changes.

Std changes are in the last commits, the extra 3 commits are the requested changes after the CI failure. I can squash them once they are approved.

Also, squash with --keep-base? It's been a while.

@rustbot ready

library/std/src/num/f32.rs

library/std/src/num/f64.rs

RalfJung · 2025-08-01T07:32:09Z

library/std/src/num/f32.rs

    ///
    /// // asin(sin(pi/2))
    /// let abs_difference = (f.sin().asin() - std::f32::consts::FRAC_PI_2).abs();
    ///
-    /// assert!(abs_difference <= 1e-3);
+    /// assert!(abs_difference <= 1e-1);


What is going on here? Surely we can do better than 1e-1?!

These faulty tests were used when finding the limits. I have changed them accordingly.

rustbot · 2025-08-03T10:35:18Z

⚠️ Warning ⚠️

This PR is based on an upstream commit that is 15 days old.

It's recommended to update your branch according to the rustc-dev-guide.

RalfJung · 2025-08-03T15:22:01Z

library/std/src/num/f32.rs

@@ -1095,7 +1095,7 @@ impl f32 {
    ///
    /// let abs_difference = (f - x).abs();
    ///
-    /// assert!(abs_difference <= 1e-6);
+    /// assert!(abs_difference <= 1e-5);


Why did this change again? It's super hard to keep track when you just keep doing random-looking changes without explanation...

Excuse me, it shouldn't. I should not be implementing this while sick :).

I confused it with cosh, which is a foreign_item that should be changed in this pr, I just thought I forgot this one. I should've mentioned it.

So, does 1e-6 work here or not?

Get well soon. :)

According to the many-seeds runs on the extracted doctests it should change to 1e-5, but it did pass CI multiple times, while asin doesn't. You mentioned "minimizing the diff" if previous values already worked.

Right, and minimizing the diff would be 1e-6 if that worked. Now you changed it to 1e-5 in the last commit. Or did I misread?

rustbot assigned tgross35 Jul 13, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 13, 2025

rustbot assigned RalfJung and unassigned tgross35 Jul 13, 2025