Skip to content

f64::round doesn't work properly on arm-unknown-linux-gnueabi #122294

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

Open
vklachkov opened this issue Mar 10, 2024 · 7 comments
Open

f64::round doesn't work properly on arm-unknown-linux-gnueabi #122294

vklachkov opened this issue Mar 10, 2024 · 7 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status I-miscompile Issue: Correct Rust code lowers to incorrect machine code I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@vklachkov
Copy link

I'm cross-compiling for target arm-unknown-linux-gnueabi, and I encountered that f64::round() does not work correctly. I tried this code:

fn main() {
    let value: f64 = 15.44;
    dbg!(value);
    dbg!(value.round());
}

I expected to see:

[bug/src/main.rs:3:5] value = 15.44
[bug/src/main.rs:4:5] value.round() = 15.0

But instead, in the debug build I see:

[bug/src/main.rs:3:5] value = 15.44
[bug/src/main.rs:4:5] value.round() = 0.0

In the release build, as a rule, the output is correct, but sometimes f64::round() returning 0 or 1 instead of the correct rounding. I can't reliably reproduce this bug in release build.

I build this code in docker container. See more details under the spoiler.

Build details

I build code with this command:

docker build -f Dockerfile -t build_image .

docker run \
  -v $HOME/.cargo/registry:/root/.cargo/registry \
  -v $(pwd):/src \
  -t build_image

Dockerfile:

FROM debian:11-slim

# Install Rust
RUN apt-get update && apt-get install -y curl
RUN curl https://sh.rustup.rs -sSf | bash -s -- -y
ENV PATH="/root/.cargo/bin:${PATH}"

# Install tools
RUN apt-get update && apt-get install -y \
    crossbuild-essential-armel

# Install target for ARMv6
RUN rustup target add arm-unknown-linux-gnueabi

# Build
RUN mkdir -p /src
WORKDIR /src

# Remove --release for debug build
CMD cargo build --release --target=arm-unknown-linux-gnueabi

Cargo.toml:

[package]
name = "bug"
version = "0.1.0"
edition = "2021"

[dependencies]

[profile.release]
opt-level = 3
strip = "debuginfo"
panic = "abort"

.cargo/config.toml:

[target.arm-unknown-linux-gnueabi]
linker = "arm-linux-gnueabi-gcc"
rustflags = ["-L", "/usr/lib/arm-linux-gnueabi"]

binaries.tar.gz
source.tar.gz

Meta

rustc --version --verbose:

rustc 1.76.0 (07dca489a 2024-02-04)                                                                                                                                                           
binary: rustc                                                                                                                                                                                 
commit-hash: 07dca489ac2d933c78d3c5158e3f43beefeb02ce                                                                                                                                         
commit-date: 2024-02-04                                                                                                                                                                       
host: x86_64-unknown-linux-gnu
release: 1.76.0
LLVM version: 17.0.6
@vklachkov vklachkov added the C-bug Category: This is a bug. label Mar 10, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 10, 2024
@saethlin saethlin added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Mar 11, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 11, 2024
@saethlin
Copy link
Member

With -Copt-level=0, round looks like this:

        push    {r11, lr}
        mov     r11, sp
        sub     sp, sp, #8
        bl      round
        str     r1, [sp, #4]
        str     r0, [sp]
        ldr     r0, [sp]
        ldr     r1, [sp, #4]
        mov     sp, r11
        pop     {r11, pc}

With optimizations enabled:

        push    {r11, lr}
        bl      round
        pop     {r11, pc}

I'm guessing that this is an LLVM backend-specific issue, but I don't have any 32-bit ARM on hand to test these.

@saethlin
Copy link
Member

Realizing that the problem is more likely related to compiling the float formatting machinery. We've for sure found miscompiles in there before.

@vklachkov
Copy link
Author

@saethlin

With -Copt-level=0, round looks like this

I also looked into assembler, but I still didn't understand where the round was located and what the root of the issue was.

I don't have any 32-bit ARM on hand to test these

I'm reproducing this issue on Raspberry Pi Zero, but I think this bug can be reproduced in Qemu.

Realizing that the problem is more likely related to compiling the float formatting machinery. We've for sure found miscompiles in there before.

Literally on the same day, while dealing with f64::round, I encountered another bug: #46950. Another broken floating point operation that can also be workarounded with a naive implementation.

Can I help with this bug?

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Mar 12, 2024
@saethlin
Copy link
Member

Can I help with this bug?

Maybe. Usually the best thing to do here is to minimize the reproducer. The fact that your reproducer uses the standard library formatting is a bit vexing, because if that's where the code pattern is that's being miscompiled, you have a lot of code to start from.

There are other formatting crates, so I'd see if this reproduces with one of them. It's pretty easy to find a handful of them by searching crates.io. Then I'd download the source and start minimizing it as much as you can while retaining the "different behavior in debug and release on 32-bit ARM" property.

@vklachkov
Copy link
Author

vklachkov commented Mar 14, 2024

@saethlin

I decided not to try to make a minimal repro. I was afraid that any “my” code could introduce new bugs. And I went looking for a very simple solution, where it would be clearly visible that the problem was in f64::round().

I abandoned std format and used ryu and libc::write. I know for sure that ryu and libc::write work flawlessly: they pass all tests, errors in these libraries would have been noticed long ago.

As a result, I wrote this repro:

Code
fn main() {
  // Debug fail, release ok:
  // print_float(15.44f64);
  // print_float(15.44f64.round());

  // Debug fail, release fail:
  // print_float(std::hint::black_box(get_float()));
  // print_float(std::hint::black_box(get_float()).round());
}

#[inline(never)]
fn get_float() -> f64 {
  15.44
}

#[inline(never)]
fn print_float(value: f64) {
  let mut buffer = ryu::Buffer::new();
  let printed = buffer.format(value);

  print_str(printed);
  print_str("\n");
}

#[inline(always)]
fn print_str(s: &str) {
  unsafe { libc::write(1, s.as_ptr() as _, s.len()) };
}

The difference between the first two lines and the second two lines is that in the first case Rust optimizes f64::round, but in the second it does not. Without optimizations, I see that “bl round” returns invalid value.

To be even more confident, I abandoned rye altogether and simply decided to print the bytes of the rounded float:

No float format code
fn main() {
  print_float(std::hint::black_box(get_float()));
  print_float(std::hint::black_box(get_float()).round());
}

#[inline(never)]
fn get_float() -> f64 {
  15.44
}

#[inline(never)]
fn print_float(value: f64) {
  let printed = format!("{:X?}", value.to_ne_bytes());
  print_str(&printed);
  print_str("\n");
}

#[inline(always)]
fn print_str(s: &str) {
  unsafe { libc::write(1, s.as_ptr() as _, s.len()) };
}

And got zeroes:

[E1, 7A, 14, AE, 47, E1, 2E, 40]
[0, 0, 0, 0, 0, 0, 0, 0]

If necessary, I will post this code on Github and attach binaries.

I don't know what the problem is with f64::round, but I see that std format has nothing to do with it.

@jieyouxu jieyouxu added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 17, 2024
@workingjubilee workingjubilee added the I-miscompile Issue: Correct Rust code lowers to incorrect machine code label Oct 2, 2024
@jieyouxu
Copy link
Member

P-high pre-triage: tagging this as E-needs-investigation as this might need a bit more investigation to determine if this is a miscompile from rustc opts or LLVM backend problem.
@rustbot label +E-needs-investigation

@rustbot rustbot added the E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-investigation Call for partcipation: This issues needs some investigation to determine current status I-miscompile Issue: Correct Rust code lowers to incorrect machine code I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants