laurencelundblade / QCBOR

Comprehensive, powerful, commercial-quality CBOR encoder/ decoder that is still suited for small devices.
Other
182 stars 47 forks source link

Add compile time option to disable floating point #114

Closed matetothpal closed 3 years ago

matetothpal commented 3 years ago

This pull request adds the compile time option to disable floating point functionality in QCBOR (i.e. to conditionally compile floating point related code). Doing this compile time (in contrast to doing this linker time, as it is currently possible) can be necessary if the project using the QCBOR library has the requirements to use only general purpose registers only.

See GCC 6.31.2 AArch64 Function Attributes:

general-regs-only Indicates that no floating-point or Advanced SIMD registers should be used when generating code for this function. If the function explicitly uses floating-point code, then the compiler gives an error. This is the same behavior as that of the command-line option -mgeneral-regs-only.

The pull request also contains two other commits to add some extra builds to the CI to exercise the defines that make disabling/restricting floating point functionality of QCBOR.

laurencelundblade commented 3 years ago

Still thinking, but a few comments.

See https://github.com/laurencelundblade/qdv for the scripts I use to test and develop. I keep them separate to not impose the long build and test times on normal users. This fans out all combinations of ifdefs.

There is -DQCBOR_DISABLE_FLOAT_HW_USE, but it doesn't completely eliminate the use of the type double and float. When it is defined QCBOR will not do anything operation with a double or float other than return the type.

When I use GCC to compile with both -mgeneral-regs-only -DQCBOR_DISABLE_FLOAT_HW_USE I get errors, so probably your change is needed to be able to use -mgeneral-regs-only.

I need to check on what LLVM does.

Why would one set -mgeneral-regs-only? Presumably the compiler knows if the HW has floating point registers and will never generate code that uses them because it would be incorrect. On the other hand, if the HW has those registers and code is generated that uses them, but is never executed, what's the harm?

This is a fairly messy ifdef affecting interfaces and such. No other ifdef affects interfaces. I want to be sure this is really important.

matetothpal commented 3 years ago

Thanks for looking at the PR.

I didn't know about these scripts, They look really useful. I guess I'm OK to remove the CI related commits from this PR then, and use the scripts for testing myself.

if QCBOR needed to be used in supervisory software, typically the supervisory software disables usage of FPU due to memory foot print /latency complications. If the supervisory software needs to have FPU support enabled, then it makes the implementation complicated due to the way it manages the FPU context for user applications.

Using -mgeneral-regs-only gives the extra safety that the compiler refuses to generate code that uses FPU registers, compared to relying on the linker eliminating floating point related code. In addition to that if the linker is used for this purpose there is no way to detect if accidentally some FPU using code is referenced and hence not eliminated.

Indeed the change is quite messy. I also understand the concerns related to the ifdefs in the interface. One way to overcome this (not sure if it is possible at all) might be to organize floating point related functionality to separate c/h file. Again, not sure whether this could be done smoothly at all.

laurencelundblade commented 3 years ago

Thank you for the work here.

Still thinking about this, but am coming to think the change is necessary.

I understand your comment about supervisor mode. It makes sense.

I also found this when trying to figure out what LLVM does in this regard. https://reviews.llvm.org/D40256

laurencelundblade commented 3 years ago

So what happens if you give -mfloat-abi=soft?

This appears to work for both llvm and gcc.

It results in SW floating point being used in the ABI and for any actual calculation. If you use this with -DQCBOR_DISABLE_FLOAT_HW_USE it should result in QCBOR not using any floating point registers in any way and then be OK with the -mgeneral-regs-only flag.

With -DQCBOR_DISABLE_FLOAT_HW_USE QCBOR will still copy floating point numbers around, but it will never try to add, multiply or such with them. With the -mfloat-abi=soft that copying should always be in integer registers (except for AArch64 architectures).

I'm thinking to recommend -mfloat-abi=soft plus -DQCBOR_DISABLE_FLOAT_HW_USE QCBOR rather than the big set of ifdefs to eliminate all use of double, float...

https://wiki.segger.com/GCC_floating-point_options https://wiki.debian.org/ArmHardFloatPort/VfpComparison https://reviews.llvm.org/D40256

tamasban commented 3 years ago

Thanks for the proposal!

We will investigate it. Currently, both of us (Mate&me) is on leave. We will come back with the results in a few weeks.

laurencelundblade commented 3 years ago

Also, look at the new version of the README.md. It has lots more info on floating point, though it may not be the solution.

Generally, I think you would want to define QCBOR_DISABLE_FLOAT_HW_USE QCBOR and QCBOR_DISABLE_PREFERRED_FLOAT for the TFM use case.

matetothpal commented 3 years ago

Sorry for the long delay, and thanks for looking into this problem further.

The reason we can't use -mfloat-abi=soft is just as you mentioned above: that option is not available for AArch64 compilers. The reason is that there is no soft floating point ABI defined for that architecture. This means that the +nofp and +nosimd modifiers won't work either for the -mcpu and -march options.

So to restrict the usage of FP registers on AArch64 the only option seems to be -mgeneral-regs-only option, which unfortunately also restrict the usage of FP in the source code.

laurencelundblade commented 3 years ago

OK. Understood soft doesn't work for you. Got a few other things going on so it will take some time to get to this.

tamasban commented 3 years ago

@laurencelundblade: Just wondering whether could you take a look at the fixes?

laurencelundblade commented 3 years ago

Hi

This has kind of precipitated a general review of the floating-point error handling for me, so it's taking some time.

So far, your PR looks good. The macros you came up with for the errors seem right. Thanks for that work.

In the long term, I want to simplify the testing some more, but I will do that myself. You've done enough. :-). I plan to merge this after a little more checking.

Thx