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

(potential?) bug: AVR - Incorrect f64 handling #118079

Closed
onkoe opened this issue Nov 20, 2023 · 4 comments · Fixed by rust-lang/compiler-builtins#558 or #118645
Closed

(potential?) bug: AVR - Incorrect f64 handling #118079

onkoe opened this issue Nov 20, 2023 · 4 comments · Fixed by rust-lang/compiler-builtins#558 or #118645
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. O-AVR Target: AVR processors (ATtiny, ATmega, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@onkoe
Copy link
Contributor

onkoe commented Nov 20, 2023

Hi there! I encountered a weird situation when writing a device driver using embedded-hal. Despite getting correct temperature readings in byte form from my thermocouple converter, all readings were incorrect after casting them to f64 floats and applying some fundamental transformations.

After much confusion, I tried using f32 instead and found it worked perfectly!

However, I knew the transformations should be correct either way: I tested this exact setup and formula using Linux and the spidev crate.

That led me to test those transformations on a few different machines. Please take a look at the results below to gain better insight.

If there's any additional context I can give, or checking to be done, please let me know, and I'll happily look into it!

Also, I did check the ufmt_float crate to see if it was working properly! I assume that since the f64 as u32 and ufmt_float'd f64are the same, it must be an issue with f64 itself. Please see the source and serial output of the original test to get more info.

Thank you for taking a look at my problem!

@onkoe onkoe added the C-bug Category: This is a bug. label Nov 20, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 20, 2023
@saethlin saethlin added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-AVR Target: AVR processors (ATtiny, ATmega, etc.) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 20, 2023
@apiraino
Copy link
Contributor

@onkoe trying to figure out if this is a regression. I see that you tested on nightly-2023-08-08, does it also reproduce on other versions of the rust compiler? Did you try using a stable compiler (if usable for your project)?

@Patryk27
Copy link
Contributor

Patryk27 commented Nov 24, 2023

AVR doesn't support 64-bit floats in Rust (operations on them should end up being linker errors, though 👀):
rust-lang/compiler-builtins#527

The fundamental issue is that AVR intrinsics (e.g. operations for i32 or f32) require a special calling convention that hasn't been yet exposed from LLVM, preventing Rust's compiler-builtins from chiming in with its own implementation.

Some intrinsics (such as those for i32, u32, f32 etc.) get implemented by linking the resulting AVR binaries with hand-written functions from GCC (that's why working with AVR requires avr-gcc), but GCC doesn't provide implementations for f64.

tl;dr f64 is not supported, but (probably) should throw a linker error instead of trying to link with compiler-builtin's implementation (because that implementation uses a different calling convention than the LLVM's codegen expects)

Edit: GCC does seem to contain proper impls nowadays (since 2020, actually) - maybe LLVM gets the calling convention wrong? I'll take a look over the weekend.

Patryk27 added a commit to Patryk27/compiler-builtins that referenced this issue Nov 26, 2023
Tale as old as the world - there's an ABI mismatch:
rust-lang#527

Fortunately, newest GCCs (from v11, it seems) actually provide most of
those intrinsics (even for f64!), so that's pretty cool.

(the only intrinsics not provided by GCC are `__powisf2` & `__powidf2`,
but our codegen for AVR doesn't emit those anyway.)

Fixes rust-lang/rust#118079.
@Patryk27
Copy link
Contributor

Patryk27 commented Nov 26, 2023

Alright, got it! -- fix at: rust-lang/compiler-builtins#558.

(note that merging that merge request in compiler-builtins doesn't yet solve the issue here, because then I'll need to prepare another merge request bumping compiler-builtins inside rustc - all should be ready within a few days)

As for the cause - since AVRs don't have a native support for floating-point operations, those must be implemented in software; usually implementations for things like that end up in a crate called compiler-builtins (linked above), which implements them in Rust using simple bitwise operations and whatnot.

There's a difference, though, between the ABI as expected by LLVM and the code that's present inside compiler-builtins - for "bigger" types (like i64, i128, f32 etc.), AVR requires a special calling convention (which defines things like "which register should contain which value") that is not yet supported by compiler-builtins (and not so easy to expose there).

So instead, we pull hand-written functions ("intrinsics") from GCC's standard library (which is why avr-gcc is required as the linker) - those functions use the correct registers -- but the linking order is that stuff from compiler-builtins gets linked before GCC's, that's why those invalid intrinsics have to be commented out in compiler-builtins (so that the avr-gcc can provide its own, correct definitions).

This is exactly the same case as we used to have for 64-bit integers etc.

@onkoe
Copy link
Contributor Author

onkoe commented Nov 26, 2023

Woah, thanks for the fix! Sorry for not updating this issue for a bit - my wired internet hasn't been working, so I haven't had access to my Linux machine to test this functionality.

Thank you for taking the time to get it working (and explain it to me)! 🤩

GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this issue Jan 9, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jan 9, 2024
…=Nilstrieb,dtolnay

chore: Bump compiler_builtins

Actually closes rust-lang#118079.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jan 9, 2024
Rollup merge of rust-lang#118645 - Patryk27:bump-compiler-builtins, r=Nilstrieb,dtolnay

chore: Bump compiler_builtins

Actually closes rust-lang#118079.
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. O-AVR Target: AVR processors (ATtiny, ATmega, etc.) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants