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

MIPS: println! before return statement changes returned value #35673

Closed
ghost opened this issue Aug 15, 2016 · 8 comments
Closed

MIPS: println! before return statement changes returned value #35673

ghost opened this issue Aug 15, 2016 · 8 comments

Comments

@ghost
Copy link

ghost commented Aug 15, 2016

Hi all,
Asked about this in rust-beginners and was advised it's probably a bug. On MIPS (specifically, on the Tessel 2, technical overview) I'm getting different (incorrect) results from a function that does a bit of math, depending on:

  • if I'm accessing a struct variable from self
  • if I'm taking method arguments
  • if and where the println!'s are in the method
  • the size of the primitives I'm using to calculate each intermediate part (not an issue on my OS X desktop or the playground)

Here's a playground link with my ideal variation of the method; ideally, it should compute baud(9600) = 65326. On MIPS, depending on the various factors I've listed above, I've seen varying results including 0 and 4294967295.

From what I can tell, this is falling apart with the floor() call (which appears to be an intrinsic - something I've seen others have difficulty with on MIPS). Take this snippet:

        // wanted_f: f64 = 9600;
        let part1: f64 = wanted_f / 48_000000f64;
        let part2: f64 = 16f64 * part1;
        let part3: f64 = 1f64 - part2;
        let part4: f64 = 65536f64 * part3;
        let part5: f64 = part4.floor();
        let part6: u32 = part5 as u32;

        println!("1: {}", part1);
        println!("2: {}", part2);
        println!("3: {}", part3);
        println!("4: {}", part4);
        println!("5: {}", part5);
        println!("6: {}", part6);

Gives the output:

1: 0.0002
2: 0.0032
3: 0.9968
4: 65326.2848
5: 0.0002
6: 0

(where 5: should be 65326). If I add another intermediate local in between part4 and part5 (let inter: f64 = part4.floor()), both inter and part5 have the 0.0002 value when printed.

However, I can seemingly coerce it into working by putting a println! in the right place:

        let wanted_f: f64 = self.baudrate as f64;
        let baud: f64 = 65536f64 * (1f64 - (16f64 * (wanted_f / 48_000000f64)));
        println!("inside fn: {}", baud);
        return baud.floor() as u32

This returns the correct result, even on MIPS.

@ghost
Copy link
Author

ghost commented Aug 15, 2016

Also, seems like I can make the problem go away by making my own floor():

    fn baud(&mut self) -> u32 {
        // baud is given by the following:
        // baud = 65536*(1-(samples_per_bit)*(f_wanted/f_ref))
        // samples_per_bit = 16, 8, or 3
        // f_ref = 48e6
        let wanted_f: f64 = self.baudrate as f64;
        let baud: f64 = 65536f64 * (1f64 - (16f64 * (wanted_f / 48_000000f64)));

        // https://github.com/rust-lang/rust/issues/35673
        // floor(n) = n - (n % 1)
        (baud - (baud % 1f64)) as u32
    }

.. computes the correct result.

@Aatch
Copy link
Contributor

Aatch commented Aug 15, 2016

Hmm, might be a constant-folding issue.

I made a C version of the code and it produces the same ASM as the Rust version on x86. A function that just supplies 9600 to baud is constant folded to the expected value. I also looked at the MIPS output from clang to see if anything was wrong there, but it looks fine. Furthermore, the constant-folding is correct here.

Could you post the MIPS ASM of this please:

#![crate_type="lib"]

pub fn baud(wanted: u32) -> u32 {
    // baud is given by the following:
    // baud = 65536*(1-(samples_per_bit)*(f_wanted/f_ref))
    // samples_per_bit = 16, 8, or 3
    // f_ref = 48e6
    let wanted_f: f32 = wanted as f32;
    let baud: f32 = 65536f32 * (1f32 - (16f32 * (wanted_f / 48_000000f32)));
    baud.floor() as u32
}

pub fn baud2() -> u32 {
    baud(9600)
}

As setting up a MIPS toolchain is proving difficult.

@nagisa
Copy link
Member

nagisa commented Aug 15, 2016

Are you sure the float ABI between Rust and system libc matches? If the floor intrinsic lowers down to a libc call and the ABI mismatches, then you’ll see weird results like these with any non-native float op.

Things to try: -C soft-float.

@nagisa
Copy link
Member

nagisa commented Aug 15, 2016

See #34967 and #34910 and #34967 which are all considerably recent changes to MIPS targets’ float ABI.

@japaric
Copy link
Member

japaric commented Aug 16, 2016

I was checking the tessel-rust repository and saw this line:

OpenWrt-SDK-ramips-mt7620_gcc-4.8-linaro_uClibc-0.9.33.2.Linux-x86_64.tar.bz2

The target system is running uclibc. mips-unknown-linux-gnu targets glibc-based mips systems. This combination is not guaranteed to work (You are actually lucky it has been working so far). There is also the issue that @nagisa mentioned: this target now produces hard float ABI code, the toolchain (the C libraries in it) adds soft float ABI code; this combination is a recipe for problems -- one must not mix code with different float ABIs.

In conclusion, you want to create and use a mips-unknown-linux-uclibc target that's tailored for these platforms (OpenWRT 15.05 platforms).

The other option is to run OpenWRT trunk, which is musl based, on the Tessel, then you can, instead, use the mips-unknown-linux-musl target, which already exists.

@ghost
Copy link
Author

ghost commented Aug 16, 2016

Hey all, thanks so much for all the investigation and deep dive into this. One of the Tessel developers also pointed me to #34906 with the same findings. Please feel free to resolve, I'll see about updating the target. Thanks again!

japaric pushed a commit to japaric/rust that referenced this issue Aug 16, 2016
These targets cover OpenWRT 15.05 devices, which use the soft float ABI
and the uclibc library. None of the other built-in mips targets covered
those devices (mips-gnu is hard float and glibc-based, mips-musl is
musl-based).

With this commit one can now build std for these devices using these
commands:

```
$ configure --enable-rustbuild --target=mips-unknown-linux-uclibc
$ make
```

cc rust-lang#35673
eddyb added a commit to eddyb/rust that referenced this issue Aug 18, 2016
add mips-uclibc targets

These targets cover OpenWRT 15.05 devices, which use the soft float ABI
and the uclibc library. None of the other built-in mips targets covered
those devices (mips-gnu is hard float and glibc-based, mips-musl is
musl-based).

With this commit one can now build std for these devices using these
commands:

```
$ configure --enable-rustbuild --target=mips-unknown-linux-uclibc
$ make
```

cc rust-lang#35673

---

r? @alexcrichton
cc @felixalias This is the target the rust-tessel project should be using.
Note that the libc crate doesn't support the uclibc library and will have to be updated. We are lucky that uclibc and glibc are somewhat similar and one can build std and even run the libc-test test suite with the current, unmodified libc. About that last part, I tried to run the libc-test and got a bunch of compile errors. I don't intend to fix them but I'll post some instruction about how to run libc-test in the rust-lang/libc issue tracker.
@japaric
Copy link
Member

japaric commented Aug 19, 2016

Appropriate targets for this use case (OpenWRT 15.05 on MIPS, e.g. Tessel 2) landed in #35734. Proper support for these targets in libc is being tracked in rust-lang/libc#361, which contains instructions about how to update libc and run its test suite.

Once libc is updated perhaps we can start building binary releases of std for these targets. cc @alexcrichton

@Mark-Simulacrum
Copy link
Member

rust-lang/libc#361 is still open. As I understand it, this specific issue isn't tracking more than "build binary releases of std" which isn't something we currently want to track as far as I know, so I'm going to close this issue. If people are interested, please ask on internals.rust-lang.org for instructions on how to add a platform to Rust's standard builds.

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

4 participants