Add new implementation of math library (ibmcs), remove math library dependency from strtod.c by ayoopierre · Pull Request #438 · phoenix-rtos/libphoenix

ayoopierre · 2025-09-22T14:37:52Z

Add new implementation of math library (libmcs).

Description

Add new implementation of math library (libmcs), move libphoenix implementation of math library, remove dependency of strtod.c on math library. Current configuration will fail to compile on ia32 because of patches on micropython applied before compilation.

Motivation and Context

Adding complete math library implementation with extensions for critical applications

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

All projects, that depend on math library should add appropriate flag for linking against new library

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (all targets except for ia32 - new library causes compilation error due to patches on micropython).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

gemini-code-assist · 2025-09-22T14:38:09Z

Summary of Changes

Hello @ayoopierre, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the libphoenix project by decoupling its math library implementation. The math-related code, including complex.h and math.h headers, has been moved out to a new core-libs project. Consequently, strtod.c has been updated to remove its direct dependency on math.h and now utilizes GCC built-in functions for floating-point operations, ensuring continued functionality without the removed headers. The build system has also been adjusted to reflect these changes.

Highlights

Math Library Decoupling: The libphoenix math library implementation has been removed from this project, as it is now expected to reside in a separate core-libs project.
strtod.c Refactoring: The stdlib/strtod.c file no longer depends on math.h and has been updated to use GCC built-in functions (__builtin_inff, __builtin_nanf, __builtin_isinf_sign) for floating-point constants and checks.
Build System Update: The main Makefile has been modified to remove the inclusion of the math/Makefile, reflecting the removal of the in-tree math library.
Header Removal: The include/complex.h and include/math.h header files have been removed from the libphoenix project.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request removes the dependency on the local math.h implementation by deleting the math-related headers and updating strtod.c to use compiler built-ins. The changes are generally good, but there are some issues with type correctness when assigning infinity and NaN values to long double variables. I've pointed out several places where __builtin_inff() (for float) or __builtin_inf() (for double) are used instead of the correct __builtin_infl() for long double. This can lead to precision loss or incorrect behavior. Please address these to ensure type safety.

gemini-code-assist · 2025-09-22T14:39:10Z

stdlib/strtod.c

 		}

-		*result = (*sign) * INFINITY;
+		*result = (*sign) * __builtin_inff();


The result variable is a pointer to long double, but __builtin_inff() returns a float infinity. This can lead to incorrect value assignment or loss of precision. You should use __builtin_infl() which returns a long double infinity to match the type of *result.

*result = (*sign) * __builtin_infl();

gemini-code-assist · 2025-09-22T14:39:10Z

stdlib/strtod.c

 		}

-		*result = NAN;
+		*result = __builtin_nanf("");


The result variable is a pointer to long double, but __builtin_nanf("") returns a float NaN. To ensure type correctness, you should use __builtin_nanl("") which returns a long double NaN.

*result = __builtin_nanl("");

gemini-code-assist · 2025-09-22T14:39:10Z

stdlib/strtod.c

 		}
 		else if (total_exp > exp_max) {
-			result = INFINITY;
+			result = __builtin_inff();


The result variable is of type long double, but __builtin_inff() returns a float infinity. This mismatch can cause issues. Please use __builtin_infl() to get a long double infinity.

result = __builtin_infl();

gemini-code-assist · 2025-09-22T14:39:10Z

stdlib/strtod.c

 #if LDBL_MAX_10_EXP < (1 << POW10_LOOKUPS)
 	if (exp >= (1 << POW10_LOOKUPS)) {
-		return negative ? 0 : INFINITY;
+		return negative ? 0 : __builtin_inf();


The function mul_exp10 returns a long double, but __builtin_inf() returns a double infinity. For type consistency and to avoid potential issues, it's better to use __builtin_infl() which returns a long double infinity.

return negative ? 0 : __builtin_infl();

github-actions · 2025-09-29T16:08:47Z

Unit Test Results

9 462 tests +16 8 873 ✅ +16 50m 29s ⏱️ + 1m 50s
561 suites +16 589 💤 ± 0
1 files ± 0 0 ❌ ± 0

Results for commit a285919. ± Comparison against base commit 3f0d106.

♻️ This comment has been updated with latest results.

lukileczo · 2025-09-30T08:18:16Z

2 things regarding splitting this into commits:

We're aiming for every commit to be buildable.
The addition of libmcs should be a separate commit. This commit shouldn't include any other changes in our code or libmcs code, just a pure copy of upstream.

Also it's worth noting that libmcs doesn't set errno and enforces the use of math_errhandling as MATH_ERREXCEPT - if we want to keep the tests passing when this library is used, the errno checks should probably be wrapped with #ifdef on math_errhandling

nalajcie

Pointed out just a few findings. Other notes:

I don't know which version (and from where) we've imported the code. At least add some info in the commit (+maybe something like README.phoenix where you describe the imported version and eventual modifications + some rationale)
the build is still not splot into libc.a / libm.a
libm/math -> libm/phoenix ?
maybe consider forking libmcs instead if there will be some modifications to be made, I'm also not sure about importing all of the docs here
why can't we use math headers in strtod?

nalajcie · 2025-09-30T10:44:04Z

Makefile

 	mkdir -p "$(HEADERS_INSTALL_DIR)"; \
-	cp -a include/* "$(HEADERS_INSTALL_DIR)";
+	cp -a include/* "$(HEADERS_INSTALL_DIR)"; \
+	cp -a libm/libmcs/libm/include/* "$(HEADERS_INSTALL_DIR)"


always installing libmcs headers?

Also - installing a global heder like config.h is disaster-waiting-to-happen. Don't.

As discussed, with the @lukileczo and @agkaminski, we have decided that libmcs headers should be installed at all times since they provide complete C99 set of function declarations, constants and macros, and If a need arises we will add Phoenix implementation of function that are currently not implemented in libphoenix. Also having multiple versions of header is a major headache due to installation in the toolchain. The config.h file will be removed, this requires some modification to the source files.

no trace of this in any commit message or any file. How other developers could learn about it?

If there is no possibility to use the old headers - they are dead code? So - should they be removed?
We have some internal projects which depend on current libm implementation (and possibly header files) - maybe it's worth to check if they will still compile/work as expected?

Old header files have been removed, also libphoenix implementation has been modified for compatibility, with new headers (since there are no longer macros for sinf for instance, wrappers have been added). Apps such as voxeldemo, rotating rectangle or top that depended on math library seem to build just fine. Information about state of libphoenix implementation and compatibility with new headers has been placed in README.

nalajcie · 2025-09-30T10:46:50Z

libm/Makefile

+DEFINITIONS = $(filter-out #define, $(GET_DEFINITIONS))
+
+# Setting sizes of floating point types
+TARGET_NAME := __SIZEOF_DOUBLE__


don't use TARGET if not related to TARGET architecture/subarch/device

nalajcie · 2025-09-30T10:47:15Z

libm/Makefile

+LIBMCS_WANT_DAZ := n
+LIBMCS_WANT_COMPLEX := y
+
+# Grab builitn definitions from gcc


Suggested change

# Grab builitn definitions from gcc

# Grab builtin definitions from gcc

nalajcie · 2025-09-30T10:48:13Z

libm/Makefile

+DOUBLE_SIZE = $(word 2, $(shell echo "$(DEFINITIONS)" | grep -E -o '$(TARGET_NAME)\s+(\w+)'))
+
+ifeq ($(DOUBLE_SIZE), 8)
+    CFLAGS += -DLIBMCS_DOUBLE_IS_64BITS


preprocessor flags should go to CPPFLAGS

use double-space ( ) for if-else indentation in Makefiles

not sure but maybe those tests could simply be a part of our config.h (using preprocessor directly instead of grep'ing?

not taken care of, is it possible or not?

nalajcie · 2025-09-30T10:50:01Z

libm/Makefile

+# %LICENSE%
+#
+LIBMCS_WANT_DAZ := n
+LIBMCS_WANT_COMPLEX := y


?= to be able to change on per-project basis?

nalajcie · 2025-09-30T10:50:57Z

libm/Makefile

+    # OBJS += $(addprefix $(PREFIX_O)libm/, $(patsubst %.c,%.o, math/common.c math/trig.c))
+    OBJS += $(addprefix $(PREFIX_O), $(patsubst %.c,%.o, $(patsubst %complex.c,, $(wildcard libm/math/*.c))))
+
+    ifeq ($(LIBMCS_WANT_COMPLEX), y)


LIBMCS_ define impacts non-libmcs build?

Yes, this is required for compatibility with libmcs headers, name might be changed to LIBM_WANT_COMPLEX

externally-configurable variable can have different name, the libmcs-specific one can be local and depend on the external one (with LIBM_ prefix)

probably USE_LIBMCS should also have LIBM_ prefix - and all externally-configurable variables should be described (at the top of the makefile?)

nalajcie · 2025-09-30T10:52:17Z

libm/Makefile

+    CFLAGS += -DLIBMCS_WANT_COMPLEX
+endif
+
+CFLAGS += -Ilibm/libmcs/libm/include


not sure if this should be needed.

Please note that you include this file into the main Makefile, so you're passing these options to all files being compiled (polluting CFLAGS)

Is this a major issue? Previously all header files are placed in include/ directory in libphoenix, since in the future this library is supposed to be build as a separate library, could we temporarily keep headers outside the main include directory?

ok, we can keep it, please note that when compiling libc these math headers would also be accessible (might be a good thing).

nalajcie · 2025-09-30T10:52:44Z

libm/Makefile

+    OBJS += $(addprefix $(PREFIX_O), $(patsubst %.c,%.o,$(wildcard libm/libmcs/libm/mathf/*.c)))
+    OBJS += $(addprefix $(PREFIX_O), $(patsubst %.c,%.o,$(wildcard libm/libmcs/libm/mathf/internal/*.c)))
+    OBJS += $(addprefix $(PREFIX_O), $(patsubst %.c,%.o,$(wildcard libm/libmcs/libm/mathd/*.c)))
+    OBJS += $(addprefix $(PREFIX_O), $(patsubst %.c,%.o,$(wildcard libm/libmcs/libm/mathd/internal/*.c)))


uhhh, so it's still being archived into libc.a, not a separate libm.a?

As discussed with @lukileczo and @agkaminski this is beyond scope of this PR, this would require some major changes to libphoenix Makefile, and also would probably cause a breaking change, since -lm flag would have to be added to projects using math library (currently since libm is symlinked to libphoenix this is not required, also as pointed out by @lukileczo there is some issues with installation paths of libraries in current build system)

so please add TODO/FIXME/comment. Leave some written trace for the next developer.

nalajcie · 2025-09-30T13:00:00Z

another thing - libmcs implementation currently lacks sqrt optimization (ASM) for ARM platform - no need to add it within the scope of this PR, but maybe it should be clearly stated somewhere

nalajcie · 2025-10-07T15:13:48Z

some generic issues/pointers:

we're still greping preprocesor defines in Makefile instead of using our own custom config.h for libmcs - is it not possible?
the commits should have libm prefix
missing info about imported libmCS version in commit message body
are we importing some random commit instead of a tagged version? (does this commit at least pass the CI in the upstream?)

JIRA: RTOS-1132

ayoopierre requested a review from lukileczo September 22, 2025 14:37

gemini-code-assist bot reviewed Sep 22, 2025

View reviewed changes

lukileczo requested a review from agkaminski September 23, 2025 09:44

ayoopierre force-pushed the ayopierre/add_libcsm branch 4 times, most recently from 09fa8de to d91df16 Compare September 29, 2025 14:48

ayoopierre changed the title ~~Remove strtod dependency on math.h, modify Makefile, remove math headers~~ Add new implementation of math library (ibmcs), remove math library dependency from strtod.c Sep 29, 2025

ayoopierre requested a review from nalajcie September 29, 2025 14:49

ayoopierre force-pushed the ayopierre/add_libcsm branch 3 times, most recently from 54ec7a8 to 3831e3b Compare September 30, 2025 10:58

nalajcie requested changes Sep 30, 2025

View reviewed changes

ayoopierre force-pushed the ayopierre/add_libcsm branch 2 times, most recently from 4fd26ca to 975beef Compare September 30, 2025 11:03

ayoopierre force-pushed the ayopierre/add_libcsm branch 5 times, most recently from 28fac68 to fae9abe Compare October 6, 2025 11:13

ayoopierre requested a review from nalajcie October 7, 2025 15:01

Mikolaj Matalowski added 3 commits January 12, 2026 11:49

Add libmcs math library implementation

54f3c73

JIRA: RTOS-1132

Libmcs: add Makefile and cleanup

162c277

JIRA: RTOS-1132

Move libphoenix math library implementation

a285919

JIRA: RTOS-1132

ayoopierre force-pushed the ayopierre/add_libcsm branch from fae9abe to a285919 Compare January 12, 2026 11:38

	# Grab builitn definitions from gcc
	# Grab builtin definitions from gcc

Conversation

ayoopierre commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

Uh oh!

gemini-code-assist bot commented Sep 22, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

lukileczo commented Sep 30, 2025

Uh oh!

nalajcie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayoopierre Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalajcie Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayoopierre commented Sep 22, 2025 •

edited

Loading

github-actions bot commented Sep 29, 2025 •

edited

Loading

ayoopierre Sep 30, 2025 •

edited

Loading

nalajcie Sep 30, 2025 •

edited

Loading