Skip to content

Fixes to picolibc_interface#1795

Merged
kilograham merged 2 commits intoraspberrypi:developfrom
gneverov:develop
Nov 18, 2024
Merged

Fixes to picolibc_interface#1795
kilograham merged 2 commits intoraspberrypi:developfrom
gneverov:develop

Conversation

@gneverov
Copy link
Contributor

  • 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
Contributor 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
Contributor Author

gneverov commented Sep 4, 2024

Created issue #1900 for the stdin implementation selection problem.

@kilograham kilograham merged commit d6e3fa0 into raspberrypi:develop Nov 18, 2024
Comment on lines 40 to +43
#if LIB_PICO_STDIO
return stdio_getchar();
#endif
return -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this could have been

#if LIB_PICO_STDIO
    return stdio_getchar();
#else
    return -1;
#endif

but presumably most compilers will make that optimisation automatically?

gneverov added a commit to gneverov/pico-sdk that referenced this pull request Jan 13, 2025
Address the review comment to fix conditional compilation syntax:
raspberrypi#1795 (review)
gneverov added a commit to gneverov/pico-sdk that referenced this pull request Jan 13, 2025
Address the review comment to fix conditional compilation syntax:
raspberrypi#1795 (review)
@gneverov gneverov mentioned this pull request Jan 13, 2025
kilograham pushed a commit that referenced this pull request Jan 20, 2025
Address the review comment to fix conditional compilation syntax:
#1795 (review)
will-v-pi pushed a commit to will-v-pi/pico-sdk that referenced this pull request Mar 20, 2025
Address the review comment to fix conditional compilation syntax:
raspberrypi#1795 (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