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

Handle ISO C <cmath> fixes and add example #20

Merged
merged 11 commits into from
Jul 3, 2022
Merged

Handle ISO C <cmath> fixes and add example #20

merged 11 commits into from
Jul 3, 2022

Conversation

ckormanyos
Copy link
Collaborator

This is the first attempt to correct ISO C signatures (first mentioned in #17) and specifically treated in #18

@chris-durand chris-durand self-requested a review May 9, 2022 10:38
@ckormanyos
Copy link
Collaborator Author

The OP in #18 is still mentioning mismatches in signatures. So this still needs further investigation.
I'm rather sure the new example will stay unchanged. But i'd like to figure out what's going on with the OP's ATMEl Studio.

Cc: @salkinium and @chris-durand and @rleh

@ckormanyos
Copy link
Collaborator Author

Any feedback on this? Positive, negative, neutral?

@chris-durand
Copy link
Member

Any feedback on this? Positive, negative, neutral?

I'll take a look at it tomorrow. I am super busy right now and couldn't find the time yet.

@ckormanyos
Copy link
Collaborator Author

take a look at it tomorrow

Thank you Chris (@chris-durand). This is not in a hurry.

There might, in fact, be a bit of discussion needed, as how best to proceed (or even if these changes are needed). I'm open for this potential discussion.

@@ -129,17 +135,18 @@ extern "C"
return ::pow(x, y);
}

bool isnanf(float x)
int isnanf(float x)
Copy link
Member

@chris-durand chris-durand May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the return types makes sense to me. Neither of the three functions isnanf, isinff and isfinitef exist in C++ headers and C specifies int as the return type.

Copy link
Collaborator Author

@ckormanyos ckormanyos May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the return types makes sense to me

I like that way of putting it. Indeed, I have struggled a lot over the years with these kinds of points.

When wrapping the STL for <cmath> it seems that a certain amount of empirical observation will always be necessary. This is because the compiler's <math.h> header file should be taken and left unchanged. The compiler supplier, however, will be advancing this C header and (hopefully) getting closer and closer to standards adherence as we progress.

The bottom line is we need to keep an eye on the evolution of <math.h> and ensure that this is properly paired with avr-libstdcpp.

src/math.cc Outdated
Comment on lines 36 to 41
#if defined(MODM_CMATH_GCC_VERSION) && (MODM_CMATH_GCC_VERSION < 110300L)
float fabsf(float x)
{
return ::fabs(x);
}
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have an avr-gcc 11.3 at hand right now. What changes with regards to fabsf in the headers supplied by avr-gcc that we have to touch it? Is it a real function instead of a macro in 11.3?

Copy link
Collaborator Author

@ckormanyos ckormanyos May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What changes with regards to fabsf in the headers...

Good question. The three subroutines that I marked as not present in math.cc for the 11.3 compiler were (and need to be) marked that way since the designers of <math.h> elected to inline these functions at the C-header level by implementing them as static inline functions in the header itself. They can no longer be separate linkable entities in a C-file.

There is no way out of this one on the 11.3 port unless one would (as i do not recommend) modify <math.h> directly.

On the other hand, it would be conceptually thinkable to deliver a math.h with avr-libstdcpp, but I would not find that step necessary quite yet.

@chris-durand chris-durand mentioned this pull request Jun 30, 2022
32 tasks
@chris-durand
Copy link
Member

chris-durand commented Jul 1, 2022

@ckormanyos We are running into the same issue of duplicate definitions of fabs and isfinitef with the updated Mac OS gcc 10.3 build. Do you know for which versions your fix needs to be applied? I have tried changing MODM_CMATH_GCC_VERSION < 110300L to MODM_CMATH_GCC_VERSION < 100300L here and strangely enough, fabsf still compiles fine with 11.2.

@ckormanyos
Copy link
Collaborator Author

ckormanyos commented Jul 1, 2022

Do you know for which versions your fix needs to be applied?

Hi Chris (@chris-durand) it's been a while since I investigated this. If I recall correctly, the trick is to ensure that the STL <cmath> header is as consistent as possible with the underlying ISO C99 definitions in the files <math.h> and (if present) <math.cc>.

As I recall, it seems like there are a few different versions of the AVR's C99 standard C-library coursing through the Open Source community. One is from avr-libc, the other seems to be from GCC itself. I believe there is also a relatively frequently-used implementation in newlib.

We need to look at your <math.h> header on the particular port you mention. It might very well be that it disagrees with the C++ declarations that are in the master branch of avr-libstdcpp at the moment.

My general philosophy in such matters is to make as many GCC versions to be as happy as possible simultaneously, and with just a few PreProcessor switches. If this means bumping the minimum version needing correction down to 10 or 9 or even lower, that might make perfect sense.

Do you have any way of posting part or all of the <math.h> header in question on the Mac OS versiion of avr-gcc that you are working on?

@chris-durand

This comment was marked as outdated.

@chris-durand
Copy link
Member

chris-durand commented Jul 1, 2022

@chris-durand
Copy link
Member

chris-durand commented Jul 1, 2022

I have checked builds of avr-gcc 10.2, 10.3, 11.2 and 12.1. 10.2 and 11.2 have the macro definitions for fabsf. 10.3 and 12.1 have real functions. So we also have to remove the definitions for gcc 10 with minor version >= 3.

@chris-durand
Copy link
Member

chris-durand commented Jul 1, 2022

The math.h definitions changed in avr-lib 2.1.0 and that's independent of the gcc version. I have replaced the gcc version check with a check for __AVR_LIBC_VERSION__ defined in <avr/version.h>.

@chris-durand
Copy link
Member

@rleh @salkinium We have to merge this before the 2022q2 release.

Copy link
Member

@chris-durand chris-durand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ckormanyos

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets do it

@ckormanyos
Copy link
Collaborator Author

Thanks folks @chris-durand, @rleh and @salkinium. For the moment, this seems to generally improve the situation regarding consistency of <math.h> and this STL-port's <cmath>.

If we need to refine any of these signatures, or if small inconsistencies that could be managed better happen to be found in the future, please let's not hesitate to correct them as we go.

@salkinium salkinium merged commit b082b1b into modm-io:master Jul 3, 2022
@chris-durand chris-durand linked an issue Jul 19, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Repair problems ISO C signatures in <cmath>
4 participants