libdragon icon indicating copy to clipboard operation
libdragon copied to clipboard

Initial picolibc support

Open asiekierka opened this issue 9 months ago • 6 comments

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:

  • [x] ~~picolibc does not yet support the mips64 CPU family~~ addressed here: https://github.com/picolibc/picolibc/pull/730
  • [x] ~~tinystdio does not implement funopen(), which precludes supporting streaming compressed assets at this time. ~~ picolibc's tinystdio now implements funopen()!
  • [x] 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 avatar May 02 '24 15:05 asiekierka

    [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

asiekierka avatar May 02 '24 17:05 asiekierka

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.

keith-packard avatar May 03 '24 00:05 keith-packard

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.

asiekierka avatar May 05 '24 09:05 asiekierka

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.

rasky avatar May 10 '24 23:05 rasky

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.

keith-packard avatar May 11 '24 00:05 keith-packard

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

asiekierka avatar Jul 06 '24 07:07 asiekierka