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

as it is, SCB::set_fpu_access_mode() is a footgun #44

Closed
whitequark opened this issue Jun 12, 2017 · 17 comments
Closed

as it is, SCB::set_fpu_access_mode() is a footgun #44

whitequark opened this issue Jun 12, 2017 · 17 comments
Milestone

Comments

@whitequark
Copy link
Contributor

Let's say FPU is disabled at entry in main() as it usually is, and you want to do some FP operations. Presto!

fn main() {
    cortex_m::interrupt::free(|cs| {
        let scb = tm4c129x::SCB.borrow(cs);
        scb.enable_fpu();

        // some FP-using code
    });
}

... or something like that. Well, not so easy. Since the code below enable_fpu changes callee-save FP registers, LLVM will try to stack those at the top of main... using the vpush instruction. Which is not available until FPU is enabled. Oops.

I don't know how to fix this.

@whitequark
Copy link
Contributor Author

The TI Tiva ROM enables FPU before entering main(). This seems like an awkward choice but maybe the cortex-m-rt crate should do the same; the FPU is not a massive power hog and power-conscious users will surely discover the option of turning it off on their own with no effort.

@edef1c
Copy link

edef1c commented Jun 12, 2017

Is there any reason that enabling the FPU before entering main couldn't be a Cargo feature flag?

@whitequark
Copy link
Contributor Author

It would be a really confusing feature flag.

@phil-opp
Copy link
Contributor

cc @oli-obk

@oli-obk
Copy link

oli-obk commented Jun 12, 2017

I don't think there can be any workarounds other than moving it before main, because the floating types are available everywhere and the moment they are used this issue occurs. A quick solution would be a lint that detects such situations, but it won't help much in the presence of optimizations.

A feature flag would not really work, since any crate also depending on the crate can forget the feature flag, thus breaking your code. This means that there's no way to write the feature in an additive way.

@whitequark
Copy link
Contributor Author

A feature flag would not really work, since any crate also depending on the crate can forget the feature flag, thus breaking your code. This means that there's no way to write the feature in an additive way.

Thanks, you've articulated my concern far better than myself.

@edef1c
Copy link

edef1c commented Jun 12, 2017

That makes sense. Is there some kind of #[cfg(hard_fp)] that could gate it based on whether hard FP is enabled in the target definition?

@whitequark
Copy link
Contributor Author

Not sure. See rust-lang/rust#32651.

@japaric
Copy link
Member

japaric commented Jun 14, 2017

Seems fine to make it a conditional feature of the cortex-m-rt crate. As per the cortex-m-quickstart model the rt crate is always a direct dependency of the application / top crate and doesn't appear anywhere else in the dependency graph (*) so the application writer always has full control over which Cargo feature of the rt is enabled / disabled.

(*) not that I can actually stop anyone from making a library depend on the rt crate but it doesn't make sense to do so since that crate has no API.

Is there some kind of #[cfg(hard_fp)] that could gate it based on whether hard FP is enabled in the target definition?

Note that there are use cases where the target has a FPU but one would prefer to keep the FPU disabled if they are not using it in the application. Always enabling the FPU if the target has a FPU would prevent that use case.

@whitequark
Copy link
Contributor Author

whitequark commented Jun 14, 2017

Note that there are use cases where the target has a FPU but one would prefer to keep the FPU disabled if they are not using it in the application. Always enabling the FPU if the target has a FPU would prevent that use case.

I strongly feel that the FPU should be always enabled if any f32/f64 parameters are passed in FP registers in the Rust ABI, because having to debug this is a horrible thing to do to your users.

I also feel that a feature of cortex-m-rt crate is not the right solution here because, so long as it's additive, it is easy to accidentally forget to activate it when setting no-default-features = true for some unrelated reason, and, see above. A subtractive feature is of course out of question.

I think that disabling the FPU right away in main(), or (more likely) fiddling with FPU somewhere within interrupt handlers is perfectly acceptable for the tiny minority of cases that truly care about the static power consumption of the FPU.

@a-shafir
Copy link

a-shafir commented Jun 14, 2017

it is easy to accidentally forget to activate it when setting no-default-features = true

#[cfg(not(no_hard_fp))]

@whitequark
Copy link
Contributor Author

@a-shafir That's a subtractive feature and is not a correct use of the system.

@sbourdeauducq
Copy link
Contributor

Note that there are use cases where the target has a FPU but one would prefer to keep the FPU disabled if they are not using it in the application.

In that case, maybe they can disable it when entering main (and we have it fully enabled by default)?

@japaric
Copy link
Member

japaric commented Jun 15, 2017

it is easy to accidentally forget to activate it when setting no-default-features = true

I wasn't thinking of making the "fpu" feature enabled by default so would always have to enable it (regardless of whether you set default-features to true or false) if you need it. "Pay for what you use".

I strongly feel that the FPU should be always enabled if any f32/f64 parameters are passed in FP registers in the Rust ABI

Hmm, that sounds reasonable. I suppose if you are not going to use the FPU at all in your application you could just use the thumbv7em-none-eabi target which doesn't emit FPU instructions and won't have the implicit enable_fpu instruction executed before main.

That also made me realize that probably the whole Fpu register block shouldn't be exposed in the API for targets that don't have a FPU (e.g. thumbv6m-none-eabi).

@whitequark
Copy link
Contributor Author

Agreed on both counts.

@japaric
Copy link
Member

japaric commented Jul 7, 2017

cortex-m-rt v0.3.0 has been released and it enables the FPU before main (on targets that have a FPU). That doesn't change the fact that set_fpu_access_mode is still a footgun if you disable the FPU and then enable it again or don't use cortex-m-rt. Anyone wants to document the proper way to use this method? I think then we can mark this as fixed.

@whitequark
Copy link
Contributor Author

Any function that runs fully or partly with the FPU disabled must not take any floating-point arguments or have any floating-point local variables. Because the compiler might inline such a function into a caller that does have floating-point arguments or variables, any such function must be also marked #[inline(never)].

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

7 participants