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

The Miri subtree was changed

cc @rust-lang/miri

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

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.

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.

🤷 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.

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.

Reopening.

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.

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.

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 ;)

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.

Failed in rollup I think (#144231 (comment)):

---- num::flt2dec::strategy::dragon::shortest_sanity_test stdout ----

thread 'num::flt2dec::strategy::dragon::shortest_sanity_test' panicked at library/coretests/tests/num/flt2dec/mod.rs:443:5:
shortest mismatch for v=ldexp_f64(1.0, 64): actual ("18446744073709535", 20), expected ("18446744073709552", 20)

---- num::flt2dec::strategy::grisu::shortest_sanity_test stdout ----

thread 'num::flt2dec::strategy::grisu::shortest_sanity_test' panicked at library/coretests/tests/num/flt2dec/mod.rs:443:5:
shortest mismatch for v=ldexp_f64(1.0, 64): actual ("18446744073709543", 20), expected ("18446744073709552", 20)

@bors r-

Hi bors @bors r-

Should this really randomize ldexp? Powers-of-2 multiplication seems like something that can be done exactly fairly easily. And indeed we are not even using host floats for that.

Yeah, that's true. This is my mistake. Looking at implementations, this is the case, the exponent is changed directly with some multiplications/additions.

Want me to edit the second commit with --keep-base?

Want me to edit the second commit with --keep-base?

Yes, please do. :)

So I did git rebase -i upstream/master --keep-base, hope that was it 😅.

@rustbot ready

Yeah that looks good :)
@bors r+

📌 Commit a3e124f has been approved by RalfJung

It is now in the queue for this repository.

@bors r-

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

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?