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

[RFC] 64-bit compatible size_t print format specifier #20182

Closed
fzi-haxel opened this issue Dec 14, 2023 · 5 comments
Closed

[RFC] 64-bit compatible size_t print format specifier #20182

fzi-haxel opened this issue Dec 14, 2023 · 5 comments

Comments

@fzi-haxel
Copy link
Contributor

fzi-haxel commented Dec 14, 2023

Description

We're currently trying to make RIOT 64-bit compatible (#20154, #19890).
While surprisingly few changes are necessary, a lot of changes are due to print formatting. Many of them can be solved with the formatting macros like PRIxPTR, but size_t remains problematic. There is a format specifier for (s)size_t (%z and %zu), but it is not always supported.
The current coding convention states:

  • When printing a size_t
    • use %u and cast the variable to (unsigned) because newlib-nano does
      not support %zu

Here is a table of the relevant types and architectures as well as the sizes for x86-64.

arch int long size_t intptr_t
C spec >=16 >= 32 >=16 - (most of the time same as size_t)
avr8, msp430 16 32 16 16
esp32, arm32, x86 32 32 32 32
x86-64 32 64 64 64

We can see that sizeof(size_t) == sizeof(int) for all currently used architectures and therefore the current coding convention is sound for these cases. But it breaks for x86-64, as it would lead to an overflow if the value is above 32 bits. For cases where we know if a given variable will always fit into 32 bits, this is fine (and should cover most of the current print statements). But if we cannot be sure that the variable will always fit into 32 bits, for example if it is referencing a memory address, this could lead to incorrect output.

I propose to change the coding convention to something that also covers 64-bit architectures, and go through the code base to implement it. Here are some suggestions:

  1. Use unsigned long/%lu instead of unsigned/%u.
    This would fix the problem on 64-bit architectures, have no impact on 32-bit architectures, but increase stack usage and runtime on 8-bit architectures.

  2. Use PRIuPTR instead of %u.
    This should work on all architectures, but can be confusing as it effectively names a different type.

  3. Add custom PRIuSIZE, PRIxSIZE, etc. format specifier macros.
    This will work on all architectures, but would be a RIOT-specific format specifier macro. Also, we would have to either create a new header or add it to an existing header (like sys/include/architecture.h), and that header would have to be included in a quite a few sources.
    (Of course, feel free to suggest different header location and/or macro names)

I would really appreciate your feedback on what is the best way to proceed.
And if we agree on a solution, I would also appreciate any support in going through the code base to implement any new formatting.

@OlegHahm
Copy link
Member

The more I think about it the more I come to the conclusion that a dedicated format specifier (i.e., option 3) seems to be the best option. Anything else is basically a workaround which either wastes memory or results in less readable code.

@maribu
Copy link
Member

maribu commented Dec 18, 2023

There is a plan to move to picolibc as default standard C lib. I think that should allow using %zu as format specifier. Maybe we can push that switch again this week. If that would add support for %zu, that would be IMO the best option.

@fzi-haxel
Copy link
Contributor Author

I searched the codebase for size_t prints and replaced them with a custom size_t print modifier (option 3).
But I can easily change them to the %z modifiers if we decide to switch to picolibc. Switching to picolib could also fix support for PRIu8 (which is still used in some places) and simplify printing of uint64_t values.

@OlegHahm
Copy link
Member

Thanks for the effort!

@fzi-haxel
Copy link
Contributor Author

PR #20194 got merged

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

3 participants