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

Dependency on fenv.h makes float HW use with libm impossible #115

Closed
DusterTheFirst opened this issue Jul 10, 2021 · 6 comments
Closed

Dependency on fenv.h makes float HW use with libm impossible #115

DusterTheFirst opened this issue Jul 10, 2021 · 6 comments

Comments

@DusterTheFirst
Copy link

The hardware float functions used from fenv.h are not portable or usable on platforms that use newlib since newlib's math library libm does not implement fenv as stated in their documentation.

The aforementioned error reporting is the supported Newlib libm error handling method. However, the majority of the functions are written so as to produce the floating-point exceptions (e.g. "invalid", "divide-by-zero") as required by the C and POSIX standards, for floating-point implementations that support them. Newlib does not provide the floating-point exception access routines defined in the standards for fenv.h, though, which is why they are considered unsupported. It is mentioned in case you have separately-provided access routines so that you are aware that they can be caused.

emphasis mine

The only way to compile this library on platforms that use libm is to disable hw float support. This is fine in any encoding use cases since hw float libraries are not used, but when decoding this can cause platforms that have hardware accelerated math.h to lose out on float functionality entirely. The specific use case where this shines is when using it in a project running the raspberry pi pico sdk which depends on newlib

Possible fixes would to be to add alternative libm compatible implementations of the dependent functions allowing the user to switch implementation with a #define, or to change the current implementation to be portable across libm and non-libm systems.

@laurencelundblade
Copy link
Owner

thanks for the comment. I will look into it, but it will take a week or so.

@laurencelundblade
Copy link
Owner

I will keep looking at it, but what happened when you defined QCBOR_DISABLE_FLOAT_HW_USE?

With QCBOR_DISABLE_FLOAT_HW_USE it should compile and run and be able to decode floating point numbers. What it can't do is convert floating point to integer, double to float and handle floating point dates. Most of these are pretty uncommon needs.

This only disables QCBOR's use of HW floating point. Your application can still use it.

Encoding and decoding CBOR floating point doesn't require floating point HW or even a library. It's essentially a cast of an int * to a float *. A good compiler should be able to do this without floating point library or HW.

@DusterTheFirst
Copy link
Author

That does make sense and fits with what I found. In my opinion the functions that require fenv.h should probbably be removed if the QCBOR_DISABLE_FLOAT_HW_USE define is added as to prevent users from accidentally using them and getting weird crashes at run time. The documentation could also explain in further detail the functions that are disabled with the addition of QCBOR_DISABLE_FLOAT_HW_USE. Other than that this seems to be less of a problem than I thought of it at first, since its just those specific niche functioins which are impacted.

@laurencelundblade
Copy link
Owner

Yes, agree with you on the QCBOR_DISABLE_FLOAT_HW_USE documentation.I tried reading the documentation (that I wrote) to understand, but had to read the source code (that I wrote) to figure it out. :-)

I believe the functions that need fenv.h will return error codes when QCBOR_DISABLE_FLOAT_HW_USE is defined. They still do useful stuff even with QCBOR_DISABLE_FLOAT_HW_USE so they shouldn't be removed.

I will update the documentation and do some more cross-checking in the next week or so.

Thanks very much for filing the issue and checking back!

@laurencelundblade
Copy link
Owner

See #116

@laurencelundblade
Copy link
Owner

Thanks for the review!

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

2 participants