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

Compiler uses f64 literals by default; both float and double are 32 bits on AVR #76

Closed
exscape opened this issue Sep 19, 2017 · 11 comments

Comments

@exscape
Copy link

exscape commented Sep 19, 2017

pub extern fn main() {
    let num = 1.0;
    if num > 0.0 {}
}

The above code (and any other code that uses f64, implicitly or explicitly) fails linking:

error: linking with `avr-gcc` failed: exit code: 1
  |
  = note: "avr-gcc" "-Os" "-mmcu=atmega328p" "-L" "/home/serenity/.xargo/lib/rustlib/avr-atmega328p/lib" "/home/serenity/rust-avr/float-bug-testcase/bug_2/target/avr-atmega328p/debug/deps/float_bug_testcase-d40b13363c5ce825.0.o" "-o" "/home/serenity/rust-avr/float-bug-testcase/bug_2/target/avr-atmega328p/debug/deps/float_bug_testcase-d40b13363c5ce825.elf" "-Wl,--gc-sections" "-L" "/home/serenity/rust-avr/float-bug-testcase/bug_2/target/avr-atmega328p/debug/deps" "-L" "/home/serenity/rust-avr/float-bug-testcase/bug_2/target/debug/deps" "-L" "/home/serenity/.xargo/lib/rustlib/avr-atmega328p/lib" "-Wl,-Bstatic" "/home/serenity/.xargo/lib/rustlib/avr-atmega328p/lib/libcore-3a5a5f891c3076ed.rlib" "-Wl,-Bdynamic" "-Wl,--gc-sections" "-flto" "-fuse-linker-plugin" "-Os" "-w"
  = note: /home/serenity/rust-avr/float-bug-testcase/bug_2/target/avr-atmega328p/debug/deps/float_bug_testcase-d40b13363c5ce825.0.o: In function `main':
          float_bug_testcase.cgu-0.rs:(.text.main+0x5e): undefined reference to `__gtdf2'
          collect2: error: ld returned 1 exit status
          
error: aborting due to previous error

__gtdf2 is used for 64-bit floats, which are not supported by avr-gcc/avr-libc.
avr-gcc has sizeof(float) == sizeof(double) == 4, so there are no 64-bits floats in AVR C/C++.

If we tell the compiler to use f32, in whatever way we choose (explicit type annotation on num or using the f32 suffix on either integer literal, the code compiles successfully.

Using f32 by default seems like the best course of action IMO, if that's a reasonable change.
Even then, though, the question about how to deal with f64 when used explicitly remains.
It could be entirely disallowed, support could be added (but that would likely involve hand-writing all the necessary 64-bit soft float code in assembly), or (ugh) f64 could be 32 bits wide.

@shepmaster
Copy link
Member

Using f32 by default seems like the best course of action IMO, if that's a reasonable change.

I'm 💯 against this. We cannot effectively fork the entire Rust ecosystem because our float literals are different from the "real" compiler, causing havoc every single time a crate that wasn't explicitly designed for AVR is used.

@shepmaster
Copy link
Member

shepmaster commented Sep 19, 2017

which are not supported by avr-gcc/avr-libc.

Again, I think I'm in the minority here, but I don't think we should forcibly adhere to what GCC does. Why not implement our our 64-bit floating point routines? I believe they will be terrible, but you don't want to really use them anyway.

avr-gcc has sizeof(float) == sizeof(double) == 4, so there are no 64-bits floats in AVR C/C++.

There's no concept as "AVR C/C++". GCC's AVR C implementation is one implementation (which is valid in the C spec), but that doesn't mean it's the only possibility.

@exscape
Copy link
Author

exscape commented Sep 19, 2017

I'm 💯 against this. We cannot effectively fork the entire Rust ecosystem because our float literals are different from the "real" compiler, causing havoc every single time a crate that wasn't explicitly designed for AVR is used.

Hmm, true enough!
I agree that supporting f64 properly is very desirable, but it's unlikely to perform well or even decently on AVR with possible issues such as bloated code size, high RAM use, high register use for function calls -- 2 x f64 is 16 registers, which doesn't even fit in the call-used registers.
If it's the default, people should be probably be made aware of these issues in one way or another (docs, at the very least).

@shepmaster
Copy link
Member

people should be probably be made aware of these issues

Absolutely.

Note that most of this has been discussed elsewhere, where we already agree to supporting i128, notably bigger than a f64.

@rubberduck203
Copy link

Hmm.. there's a pretty big difference between supporting i128 and it being the default size for a literal. I understand why you wouldn't want the default size of a float to be different from the upstream, but it will mean that everyone will always need to use the f32 typehint when working with avr. Not making the default f32 for avr-rust makes the right thing hard for users. That's rarely a Good Thing™ in my experience.

Just some food for thought from something interested in the project.

@exscape
Copy link
Author

exscape commented Sep 28, 2017

Hmm. Is it possible to find some other solution than the ones listed above?

One possibility I just thought of is having f64 as default, but having a compiler flag/crate attribute to use f32 instead, and recommending AVR projects to use that. That would leave compatibility with existing crates, but make AVR-specific code more suited for such an environment.

It's still not ideal, but neither is implementing f64 support and silently using that, or simply telling users to always type annotate.

@amckinlay
Copy link

amckinlay commented Sep 29, 2017

A compiler flag that changes ABI on a per-project basis?

@exscape
Copy link
Author

exscape commented Sep 29, 2017

Hmm no, I don't think it would change the ABI... would it have to? What I mean is that the default type where none is specified and multiple are possible (e.g. println!("{}", 1.2) would be f32 rather than f64.
All uses of f64 would remain f64, and all uses of f32 would remain f32.

@shepmaster
Copy link
Member

I also believe that the fallback case is fairly rare in practice. AFAIK, it can only happen within a single method; once you pass that number to a function which accepts a f32 or f64, it's fixed. It's only in simple cases that the fallback even kicks in, the ones I think of as "hello world" level: println!("{}", 1.2).

Maybe this is basically not a real worry whatsoever, other than the fact that we need to implement the missing support library calls.

@amckinlay
Copy link

Makes sense, literal types shouldn't affect ABI.

@dylanmckay
Copy link
Member

Closing this, we all seem to be in agreement that mem::size_of::<f64>() should equal 8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants