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

libc/stdio: Increase BUF in vfprintf.c and vfwprintf.c #1400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

VexedUXR
Copy link
Contributor

With the new %b format specifier, we need at most 64 characters.

Also, the comment says that we need enough space to write a uintmax_t in octal plus one byte, I'm not sure what that's for. I guess its implying that we need a null terminator(?), but it doesn't look like we do.

This can be easily tested by doing:

printf("%lb\n", 1ul << n); /* Where n is anywhere from 32 to 63 */

@VexedUXR
Copy link
Contributor Author

VexedUXR commented Sep 1, 2024

Thinking about this some more, we should just be able to remove the whole conditional and make BUF sizeof(uintmax_t) * CHAR_BIT

With the %b format specifier we need enough space to write a uintmax_t
in binary.
@@ -289,13 +289,9 @@ vfprintf(FILE * __restrict fp, const char * __restrict fmt0, va_list ap)
/*
* The size of the buffer we use as scratch space for integer
* conversions, among other things. We need enough space to
* write a uintmax_t in octal (plus one byte).
* write a uintmax_t in binary.
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need +2 for the 0b?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't think of that, but it looks like we print it separately:

if (ox[1]) { /* ox[1] is either x, X, or \0 */

if (ox[1]) { /* ox[1] is either x, X, or \0 */

Copy link
Member

Choose a reason for hiding this comment

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

Then don't we need +1 for NUL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, we don't.

In the case of %c, we write to it manually (or in the case of a wide char through wcrtomb). Otherwise we pass it to __ujtoa or __ultoa which both stop when they're done writing the number.
After that we just print with size which is calculated using buf + BUF - cp where cp is buf plus some offset.

When calling grouping_print, to print a number with separators, we pass a start pointer and end pointer (cp and buf+BUF), which are used to call io_print (through io_printandpad) with the correct size.

Overall the code looks like it doesn't expect a null terminator. However, I think that the comment I removed was hinting that we did, otherwise I'm not sure what that +1 would be for. Unless there's some edge case I'm missing...

FWIW, the patch also survived the cirrus-ci boot smoke test.

Copy link
Member

@bsdimp bsdimp left a comment

Choose a reason for hiding this comment

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

I think that the 0b that's prented isn't accounted for, but isn't now.

@bsdimp bsdimp self-assigned this Oct 4, 2024
@bsdimp bsdimp added the ready label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants