newlib/espidf: Add espidf_picolibc cfg for picolibc O_* flag values#5035
newlib/espidf: Add espidf_picolibc cfg for picolibc O_* flag values#5035drinkcat wants to merge 1 commit intorust-lang:mainfrom
Conversation
ESP-IDF v6.0 switched from newlib to picolibc as default C library. Picolibc uses Linux-compatible O_APPEND/O_CREAT/O_TRUNC values which differ from newlib. Add espidf_picolibc cfg to select the correct values.
| // make sure to add it to this list as well. | ||
| const ALLOWED_CFGS: &[&str] = &[ | ||
| "emscripten_old_stat_abi", | ||
| "espidf_picolibc", |
There was a problem hiding this comment.
Is the expectation that a user will set this themselves? That's probably fine, just a small surprise
There was a problem hiding this comment.
Oh, yeah, thanks for catching.
I thought we still have options on how this libc change is used, but now I'm not sure anymore if I'm correct.
I think we have 2 options, and we have not reached a conclusion yet (esp-rs/esp-idf-sys#410, and around here: esp-rs/esp-idf-sys#408 (comment) -- @ivmarkov FYI):
- Users will need to pass the config.
esp-idf-sysalready did this in the past when moving fromcfg_time32tocfg_time64. This one is a bit more annoying as technically it's still possible to choose between picolibc and newlib inesp-idf'sKconfig-- but we can detect mismatch inesp-idf-sysat build time and fail with a clear error message, so maybe this is somewhat acceptable.
I have a prototype for this and I'm sure this can work. - Add a whole new toolchain (e.g.
riscv32imc-esp-espidf-picolibc). I'm not 100% sure but I think this new toolchain could passcfg_picolibcconfig tolibc? Or do we need a different structure here (usetarget_env?)
In any case , this feels almost equally painful from user side, as they'd also need to change their build files to use that new toolchain, and we still need to implement error checking inesp-idf-systo make sureesp-idf'sKconfigmatches the toolchain used.
(oh, and I guess in both cases esp-idf-sys could automatically select the proper Kconfig option based on config option -- or toolchain -- but that's orthogonal to the problem here)
There was a problem hiding this comment.
Is the expectation that a user will set this themselves? That's probably fine, just a small surprise
TL;DR: Yes, the user is supposed to pass --cfg espidf_picolibc to rustc. :-)
Long story:
This is identical to how the existing espidf_time32 / espidf_time64 flags work.
@drinkcat Can you also put espidf_time64 in the list of options, as surprisingly it is not there?
I think we have 2 options, and we have not reached a conclusion yet (esp-rs/esp-idf-sys#410, and around here: esp-rs/esp-idf-sys#408 (comment) -- @ivmarkov FYI):
I confirm we have two options (Option 1: a Rust cfg flag; Option 2: duplicate all "espidf" Rust targets to have a picolibc variant).
However this PR is already doing Option 1. Which - for now - I feel is the slightly better overall option (see below) even if it feels a bit "hacky".
- Add a whole new toolchain (e.g.
riscv32imc-esp-espidf-picolibc).
I think the correct terminology is not "toolchain" but rather, a rust target.
The drama with adding new target(s) is that we have to add quite a few of those, not just one. I.e. we currently have:
- riscv32imc-esp-espidf
- riscv32imac-esp-espidf
- riscv32imafc-esp-espidf
- xtensa-esp32-espidf
- xtensa-esp32s2-espidf
- xtensa-esp32s3-espidf
I.e. 6 in total. We'll need another 6:
- riscv32imc-esp-espidf-picolibc
- riscv32imac-esp-espidf-picolibc
- riscv32imafc-esp-espidf-picolibc
- xtensa-esp32-espidf-picolibc
- xtensa-esp32s2-espidf-picolibc
- xtensa-esp32s3-espidf-picolibc
I'm not 100% sure but I think this new toolchain could pass
cfg_picolibcconfig tolibc? Or do we need a different structure here (usetarget_env?)
Not possible. If we go with a set of new targets (Option 2) then all branching in this PR should be done by target_env = "picolibc" (because the new targets will declare that they use picolibc instead of newlib - which is the whole benefit of going with the extra targets in the first place). Note that we might have to introduce a new env for picolibc if we go with the extra targets' option. No big deal, but just mentioning.
In any case , this feels almost equally painful from user side, as they'd also need to change their build files to use that new toolchain, and we still need to implement error checking in
esp-idf-systo make sureesp-idf'sKconfigmatches the toolchain used.
That's true, though the extra targets' approach feels cleaner.
It is just that it means much more stuff to upstream (first to rustc then to libc) so I'm not sure it is worth the hassle. At least not right now. If things change, we might reconsider/revisit.
(oh, and I guess in both cases
esp-idf-syscould automatically select the properKconfigoption based on config option -- or toolchain -- but that's orthogonal to the problem here)
Nope, I don't think this is possible. Just like the Linux kernel, also ESP-IDF is Kconfig-first (i.e. the Kconfig file is "the primary source of truth"). You can't drive Kconfig with Rust options/features/cfgs, it is the other way around - you can reason - in Rust code - what was selected in Kconfig as all selections are exposed as Rust cfgs. It is just that these automatically-generated-during-esp-idf-sys-build cfgs don't reach and cannot be used in the libc code obviously.
Hence why we are introducing a "manual" one here.
There was a problem hiding this comment.
Thanks for the detailed reply! Tricky stuff, appreciated ,-P
@drinkcat Can you also put espidf_time64 in the list of options, as surprisingly it is not there?
It's not in there because it's it's not used in libc. There's only 1 use for espidf_time32:
Line 59 in 475a519
The default was flipped in 7bd7276 by @SergioGasquez -- I assume that if picolibc becomes the "usual" flag we could also introduce some espidf_newlib_legacy or something...
There was a problem hiding this comment.
Ah yes, I forgot about that. You are completely right.
Description
ESP-IDF v6.0 switched from newlib to picolibc as default C library. Picolibc uses Linux-compatible O_APPEND/O_CREAT/O_TRUNC values which differ from newlib. Add espidf_picolibc cfg to select the correct values.
Technically, a picolibc might not belong in the newlib tree, and we could create a whole new picolibc tree structure, but these 3 values are the only ones that seem to be different, at least in esp-idf context.
Detected using a fork of SergioGasquez/libc-checks: https://github.com/drinkcat/libc-checks/actions/runs/23750132941/job/69189520202
Sources
https://github.com/picolibc/picolibc/blob/f708b0107e1a51c952f5f5e296eb09528e76ebb3/libc/include/sys/_default_fcntl.h#L56
Checklist
libc-test/semverhave been updated => no new symbol*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI => not applicable
@rustbot label +stable-nominated