Fix issue of missing sign in binary_float_op when result of rem is zero

[chenyukang]

Fixes #109567

[rustbot]

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[cbeuw]

I don't think this is the proper place to fix this. l % r should already give you the right result. There is likely something wrong in the rustc_apfloat crate, probably here, though I don't really understand how soft float works

rust/compiler/rustc_apfloat/src/ieee.rs

Lines 1003 to 1017 in f421586

	(Category::Normal, Category::Normal) => {
	while self.is_finite_non_zero()
	&& rhs.is_finite_non_zero()
	&& self.cmp_abs_normal(rhs) != Ordering::Less
	{
	let mut v = rhs.scalbn(self.ilogb() - rhs.ilogb());
	if self.cmp_abs_normal(v) == Ordering::Less {
	v = v.scalbn(-1);
	}
	v.sign = self.sign;
	let status;
	self = unpack!(status=, self - v);
	assert_eq!(status, Status::OK);
	}

[chenyukang]

yes, you're right, I didn't notice use rustc_apfloat::Float.
I think we may have a similar fix at here? I updated the code.

[scottmcm]

What part of the compiler are you specifically trying to test here?

If you're updating const eval, maybe it would make sense to add a test for this in https://github.com/rust-lang/rust/tree/master/tests/mir-opt/const_prop?

[chenyukang]

The new fix don't change mir part, so I moved the test case to tests/ui/numbers-arithmetic/.

[rustbot]

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

[RalfJung]

This does nothing. Making a const fn just means the function can be called from a const, but you are not doing that, and then it is a total NOP.

What I meant is something like

const G: f64 = -1.0 % -1.0;

Then you also don't need the feature gate.

[chenyukang]

Fixed it.
According to #109573 (comment), it's not suggested to make changes to this file? 🤔

[RalfJung]

Yeah, Rust's apfloat port has been in licensing limbo for years, meaning we have no idea how to even go about fixing bugs in it. :(

[cbeuw]

If we can't change apfloat, I suggest that @chenyukang revert to the original patch of doing the check in rustc_const_eval and get the fix merged first. It's not pretty but it's better than exposing the wrong result to users

[RalfJung]

Not sure I agree -- littering the surrounding code with checks to patch the results sounds pretty bad. FWIW this is not the only bugfix that is blocked on apfloat.

We really need to figure out the apfloat situations, and solve it properly.

[chenyukang]

seems we are blocking at this PR: #96784

[eddyb]

I posted a longer update to #55993 (comment) regarding the progress with the whole rustc_apfloat situation, but of note for this PR is that the WIP repo already has this bug fixed.

[wesleywiser]

Hi @chenyukang, thanks for the fix! As @eddyb mentioned, this has been fixed in the new version of the rustc_apfloat code that we are using as of #113843. Since that PR has landed, we've closed this one.

Thanks again for your efforts! 🙂