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

Fixes to picolibc_interface #1795

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

gneverov
Copy link

  • Don't include bindings for tinystdio if picolibc was compiled with POSIX_IO.
  • Add times() function, which seems to be missed, in the same pattern as settimeofday/gettimeofday.
  • GCC complains about taking address of void value of __tls_base.

- Don't include bindings for tinystdio if picolibc was compiled with POSIX_IO.
- Add times() function, which seems to be missed, in the same pattern as settimeofday/gettimeofday.
@kilograham
Copy link
Contributor

excellent, thank you

@kilograham
Copy link
Contributor

Can you explain the upshot of 1.? what uses of picolibc does this affect?

@kilograham kilograham added this to the 2.0.1 milestone Aug 25, 2024
@gneverov
Copy link
Author

gneverov commented Aug 31, 2024

Picolibc has an option to automatically implement stdin etc. using posix functions (read, write, etc.) When this option is on, you don't want Pico SDK to redefine stdin etc.

In the first revision of this PR, I thought the Picolibc option was called POSIX_IO, but it's actually called POSIX_CONSOLE. 🤦‍♂️ I could update this PR with the correct option name, however unlike POSIX_IO, the Picolibc build does not correctly propagate the POSIX_CONSOLE option to the picolibc.h which gets included in the file modified here. This is probably an oversight in Picolibc, and I could submit a PR to Picolibc to fix that, but there's arguably a more general problem in Pico SDK: what if the user wants to define their own implementation of stdin etc.?

There are many ways you could solve that problem. Perhaps the most robust is what Picolibc does here. So, Pico SDK would define a (strong) symbol __pico_stdin and a weak alias stdin referring to it. If the user links with Picolibc built without POSIX_CONSOLE then it just works. Otherwise if they link with a Picolibc built with POSIX_CONSOLE, they will get a duplicate definition error. Then they can resolve the error by creating their own strong stdin alias referring to __posix_stdin.

Let me know how you would like me to proceed with this PR. Should I just remove the POSIX_IO part and proceed with the two remaining trivial issues? Are you interested in another PR dealing with the multiple stdin implementation issue?

@kilograham
Copy link
Contributor

Thanks, yeah, i think it would be helpful for them to expose all the config settings.

Thus far, our interface withpicolibc and thus we've been mostly targeting whatever config comes with LLVM ETA. Since both picolibc and the SDK try to fix the same problems (both in library implementations, and in interactions with the compiler - e.g. things like thread locals), we should try to make it easy/clear which of the two is in charge in any given area.

Putting the majority of the code in pico_clib_interface is a step in the right direction, but we have a ways to go.

Maybe let's open issues for each individual config point where we have problems.

@kilograham kilograham self-assigned this Sep 3, 2024
Add return value to picolibc_getc when !LIB_PICO_STDIO
@gneverov
Copy link
Author

gneverov commented Sep 4, 2024

Created issue #1900 for the stdin implementation selection problem.

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.

2 participants