stdio/printf: add error propagation by rmikielis · Pull Request #483 · phoenix-rtos/libphoenix

rmikielis · 2026-06-15T12:20:43Z

required by POSIX - return negative value and set errno on printf error.

JIRA: RTOS-1354

Fixes phoenix-rtos/phoenix-rtos-project#1416

Description

Motivation and Context

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)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (ia32-qemu).

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

Code Review

This pull request refactors the formatting library to propagate errors by changing the 'feedfunc' signature to return an 'int' instead of 'void', and introduces a 'CHECK_FAIL' macro for early returns on failure. However, several critical issues were identified in the review: returning directly on '__bignum_shiftl' failure leaks allocated bignums, using 'CHECK_FAIL' on 'format_sprintfDecimalForm' bypasses necessary cleanup causing memory leaks, and 'format_feed' should return 'ctx->error' instead of '0' when an error is present to allow early aborts.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

github-actions · 2026-06-15T12:39:16Z

Unit Test Results

10 860 tests ±0 10 190 ✅ ±0 53m 8s ⏱️ +10s
670 suites ±0 670 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit cb07ba8. ± Comparison against base commit fb9670d.

♻️ This comment has been updated with latest results.

ziemleszcz · 2026-06-16T07:25:20Z

/gemini review

gemini-code-assist

Code Review

This pull request updates the stdio formatting library to support error propagation by changing the signature of the feedfunc callback to return an int and introducing a CHECK_FAIL macro to handle failures. The review feedback suggests ensuring that vfprintf and vprintf set errno to ENOMEM when memory allocation fails, in compliance with POSIX. Additionally, it is recommended that format_printBuffer explicitly returns 0 on success to prevent positive return values from feed (such as from putchar) from propagating as unexpected non-zero success codes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

ziemleszcz · 2026-06-16T08:06:24Z

-	putchar(c);
+
+	ret = putchar(c);
+	if (ret >= 0) {


Better to compare to EOF explicitly.

ziemleszcz · 2026-06-16T08:12:04Z

+		*n = *n + 1;
+	}
+
+	return ret;


This will return positive values returned from putchar() or -1 (EOF). Better to return 0 here and some error code on EOF.

ziemleszcz · 2026-06-16T08:17:38Z

-1 is not an error code.

ziemleszcz · 2026-06-16T08:28:55Z

 		prec = precision ? precision : 1;
 		exp = format_sprintfScientificForm(&buff, &bd_backup, flags, prec - 1, &startOffset);
 		if (exp <= DOUBLE_EXP_INVALID) {
 			ret = exp - DOUBLE_EXP_INVALID;


Not an error code.

ret value is later used in function's logic. Cutting it to 0/-1 caused CI errors. Decoding what format_sprintfScientificForm returns w/o any comments is too time consuming, so left it as it is

I don't see it. At which line is it used? format_sprintfDouble is supposed to return negative error code not some unrelated negative value like exp - DOUBLE_EXP_INVALID.

ret is used in line 840 to decide if we should call format_printBuffer or not.
Regarding format_sprintfDouble returning error code, we don't utilize it in format_parse which calls that function. We only check if it returned negative value or not. errno should be already set if error occured. format_parse only returns -1/0.

ziemleszcz · 2026-06-16T08:29:41Z

 	else if (format == 'e') {
 		ret = format_sprintfScientificForm(&buff, &bd, flags, precision, &startOffset);
 		if (ret <= DOUBLE_EXP_INVALID) {
 			ret -= DOUBLE_EXP_INVALID;


Not an error code.

ret used later in function

required by POSIX - return negative value and set errno on `printf` error. JIRA: RTOS-1354

ziemleszcz · 2026-06-17T08:15:21Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the formatting library to propagate errors by changing the feedfunc signature to return an int and introducing a CHECK_FAIL macro to handle failures. However, the review feedback highlights that the printf family functions (vfprintf, vdprintf, vsprintf, and vsnprintf) return EOF on failure without setting errno as required by POSIX. It is recommended to set errno to the negative error code returned by format_parse. Additionally, the documentation comment for format_parse should be updated to correctly reflect that it propagates negative error codes rather than returning -1 on error.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-06-17T08:18:09Z

+	if (ret != 0) {
+		return EOF;
+	}


POSIX requires printf and related functions to set errno on failure. Currently, if formatting fails due to an out-of-memory error (e.g., in bignum operations or buffer allocations), format_parse returns -ENOMEM, but vfprintf returns EOF without setting errno. We should set errno = -ret when ret is a negative error code other than -1 (which indicates the error was already handled and errno was set by the underlying I/O function).

if (ret != 0) { if (ret != -1) { errno = -ret; } return EOF; }

gemini-code-assist · 2026-06-17T08:18:10Z

+	if (ret != 0) {
+		return EOF;
+	}


POSIX requires printf and related functions to set errno on failure. Currently, if formatting fails due to an out-of-memory error (e.g., in bignum operations or buffer allocations), format_parse returns -ENOMEM, but vdprintf returns EOF without setting errno. We should set errno = -ret when ret is a negative error code other than -1 (which indicates the error was already handled and errno was set by the underlying I/O function).

if (ret != 0) { if (ret != -1) { errno = -ret; } return EOF; }

gemini-code-assist · 2026-06-17T08:18:10Z

 	ret = format_parse(&ctx, vsprintf_feed, format, arg);
 	vsprintf_feed(&ctx, '\0');

-	if (ret == 0) {
-		return ctx.n - 1;
-	}
-	else {
-		return SET_ERRNO(ret);
-	}
+	return (ret == 0) ? ctx.n - 1 : EOF;


POSIX requires printf and related functions to set errno on failure. Currently, if formatting fails due to an out-of-memory error (e.g., in bignum operations or buffer allocations), format_parse returns -ENOMEM, but vsprintf returns EOF without setting errno. We should set errno = -ret when ret is a negative error code.

Suggested change

ret = format_parse(&ctx, vsprintf_feed, format, arg);

vsprintf_feed(&ctx, '\0');

if (ret == 0) {

return ctx.n - 1;

}

else {

return SET_ERRNO(ret);

}

return (ret == 0) ? ctx.n - 1 : EOF;

ret = format_parse(&ctx, vsprintf_feed, format, arg);

vsprintf_feed(&ctx, '\0');

if (ret != 0) {

errno = -ret;

return EOF;

}

return ctx.n - 1;

gemini-code-assist · 2026-06-17T08:18:10Z

 	ret = format_parse(&ctx, vsnprintf_feed, format, arg);
 	vsnprintf_feed(&ctx, '\0');

-	if (ret == 0) {
-		return ctx.n - 1;
-	}
-	else {
-		return SET_ERRNO(ret);
-	}
+	return (ret == 0) ? ctx.n - 1 : EOF;


POSIX requires printf and related functions to set errno on failure. Currently, if formatting fails due to an out-of-memory error (e.g., in bignum operations or buffer allocations), format_parse returns -ENOMEM, but vsnprintf returns EOF without setting errno. We should set errno = -ret when ret is a negative error code.

ret = format_parse(&ctx, vsnprintf_feed, format, arg); vsnprintf_feed(&ctx, '\0'); if (ret != 0) { errno = -ret; return EOF; } return ctx.n - 1;

gemini-code-assist · 2026-06-17T08:18:10Z

+/*
+ * @return: 0 on success, -1 on any error.
+ *
+ * Functions called by format_parse() shall set `errno` when error occurs.
+ */


The comment states that format_parse returns -1 on any error, but it actually propagates negative error codes (such as -ENOMEM or -errno) returned by the underlying formatting and bignum functions. Updating the comment to reflect this will improve code clarity and maintainability.

/* * @return: 0 on success, negative error code on error. * * Functions called by format_parse() shall set `errno` when error occurs. */

ziemleszcz · 2026-06-17T08:24:57Z

What's the idea with returning EOF in ...printf() functions instead of -1?

ziemleszcz · 2026-06-17T08:39:34Z


 		if (fmt != '%') {
-			feed(ctx, fmt);
+			CHECK_FAIL(ret, feed(ctx, fmt));


Function returns -ret instead of -1 as stated by the comment and errno is not set. This applies to all CHECK_FAIL() invocations in this function.

ziemleszcz · 2026-06-17T08:58:07Z

@@ -94,12 +98,7 @@ int vsprintf(char *str, const char *format, va_list arg)
 	ret = format_parse(&ctx, vsprintf_feed, format, arg);
 	vsprintf_feed(&ctx, '\0');


Consider casting the return value to (void) and adding a comment that vsprintf_feed() always succeeds.

ziemleszcz · 2026-06-17T08:59:29Z

@@ -115,10 +114,5 @@ int vsnprintf(char *str, size_t n, const char *format, va_list arg)
 	ret = format_parse(&ctx, vsnprintf_feed, format, arg);
 	vsnprintf_feed(&ctx, '\0');


Consider casting the return value to (void) and adding a comment that vsnprintf_feed() always succeeds.

gemini-code-assist Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread stdio/format.c

Comment thread stdio/format.c Outdated

Comment thread stdio/fprintf.c

rmikielis force-pushed the rmikielis/printf_error_fix branch 2 times, most recently from 650b0ab to 6cc5fcb Compare June 15, 2026 13:32

rmikielis marked this pull request as ready for review June 15, 2026 13:38

rmikielis force-pushed the rmikielis/printf_error_fix branch from 6cc5fcb to 00d2740 Compare June 15, 2026 16:22

rmikielis requested a review from a team June 15, 2026 16:38

gemini-code-assist Bot reviewed Jun 16, 2026

View reviewed changes

Comment thread stdio/fprintf.c Outdated

Comment thread stdio/printf.c

Comment thread stdio/format.c Outdated

ziemleszcz reviewed Jun 16, 2026

View reviewed changes

stdio/printf: add error propagation

cb07ba8

required by POSIX - return negative value and set errno on `printf` error. JIRA: RTOS-1354

rmikielis force-pushed the rmikielis/printf_error_fix branch from 00d2740 to cb07ba8 Compare June 16, 2026 16:33

gemini-code-assist Bot reviewed Jun 17, 2026

View reviewed changes

ziemleszcz reviewed Jun 17, 2026

View reviewed changes

		@@ -94,12 +98,7 @@ int vsprintf(char str, const char format, va_list arg)
		ret = format_parse(&ctx, vsprintf_feed, format, arg);
		vsprintf_feed(&ctx, '\0');

		@@ -115,10 +114,5 @@ int vsnprintf(char str, size_t n, const char format, va_list arg)
		ret = format_parse(&ctx, vsnprintf_feed, format, arg);
		vsnprintf_feed(&ctx, '\0');

Conversation

rmikielis commented Jun 15, 2026 • 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 left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

ziemleszcz commented Jun 16, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

ziemleszcz commented Jun 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

ziemleszcz commented Jun 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

rmikielis commented Jun 15, 2026 •

edited

Loading

github-actions Bot commented Jun 15, 2026 •

edited

Loading