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

Initial picolibc support #535

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

Conversation

asiekierka
Copy link

@asiekierka asiekierka commented May 2, 2024

picolibc offers a number of advantages over newlib, some of which include:

  • alternate "tinystdio" stdio implementation, derived from AVR libc, with a significantly smaller code size and no iprintf/printf dual implementation problem (but this has problems, see below)
  • standardizing on compiler thread local storage over custom reentrancy structures (albeit this is not necessarily of use to libdragon at this time)
  • clearer licensing terms (removal of non-BSD-license-compatible source code)

This pull request provides initial support for picolibc, as an opt-in environment variable N64_USE_PICOLIBC=true when building the toolchain. It also provides, separately, an environment variable N64_USE_PICOLIBC_TINYSTDIO=true to enable the alternate stdio implementation. Without these environment variables, newlib is used as before, with no changes.

TODO:

  • picolibc does not yet support the mips64 CPU family addressed here: Add MIPS64 CPU family support. picolibc/picolibc#730
  • ~~tinystdio does not implement funopen(), which precludes supporting streaming compressed assets at this time. ~~ picolibc's tinystdio now implements funopen()!
  • tinystdio does not implement vasnprintf(), which precludes the stack/heap implementation of rdpq_text_printf. However, this is not necessary; it's solely an optimization strategy, and vasprintf() works just fine.

Of course, compiling with tinystdio disabled means the tinystdio-specific issues don't apply.

@asiekierka asiekierka force-pushed the feature/picolibc branch 4 times, most recently from d660a22 to e4ecd7d Compare May 2, 2024 17:16
@asiekierka
Copy link
Author

asiekierka commented May 2, 2024

    [LD] build/test.elf
      text       data        bss      total filename
    111416      67852       2968     182236 build/test.elf
# picolibc (tinystdio off)
# the difference here is at least in large part due to disabling reentrancy - which wasn't used
# by libdragon anyway, and picolibc standardizes everything on compiler TLS
    [LD] build/test.elf
      text       data        bss      total filename
     94568      67196       3480     165244 build/test.elf
# picolibc (tinystdio on)
    [LD] build/test.elf
      text       data        bss      total filename
     75496      66292       3816     145604 build/test.elf
# picolibc (tinystdio on, minimal integer-only printf)
    [LD] build/test.elf
      text       data        bss      total filename
     69064      65380       3816     138260 build/test.elf

@keith-packard
Copy link

vasnprintf is a newlib extension not even present in glibc; is there some reason vasprintf isn't sufficient for your use?
funopen is coming soon.

@asiekierka
Copy link
Author

There's no reason vasprintf is not sufficient for libdragon's usecase. vasnprintf is solely used for an optimization where a 512-byte stack buffer is allocated first, then a heap allocation is only made in the unlikely event that vsnprintf creates a larger string. This is far from essential.

@rasky
Copy link
Collaborator

rasky commented May 10, 2024

This is far from essential.

It's a hot code path though. We would have to change that to vsnprintf first and if that fails, use vasprintf, which is a pity because you've to redo the whole formatting. To be fair, the case where it doesn't fit 512 bytes is cold/unlikely so maybe it doesn't matter much.

@keith-packard
Copy link

Makes sense. I think you could pretty easily build an implementation of the vasnprintf API using vsnprintf and vasprintf which did exactly that at modest cost in code space. It seems like using standard interfaces in your application would have some modest readability benefits at least. vasnprintf is not precisely documented in the newlib code, although the semantics seem fairly obvious.

@asiekierka asiekierka marked this pull request as ready for review July 6, 2024 07:57
@asiekierka
Copy link
Author

Rebased with the latest preview branch; funopen() support integrated. This means the PR is probably ready for review.

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

Successfully merging this pull request may close these issues.

3 participants