std-related patches review

[georgik]

(Migrated from espressif/rust repo)
@igrr Patches in https://github.com/ivmarkov/rust/commits/stable provide an std-capable environment on top of ESP-IDF.

Some of these patches work around the fact that ESP-IDF is not completely POSIX-compatible.

Here is a list of things that can be fixed in ESP-IDF to reduce the number of changes we need to make in rust:

Easy changes

• add posix_memalign implementation — should be easy, ESP-IDF already has memalign function implemented
• readv, writev — simple version can be wrappers around read/write
• BSD Socket APIs: accept, bind, shutdown, getpeername, getsockname, getsockopt, setsockopt, closesocket, connect, listen, recvmsg, recv, recvfrom, sendmsg, send, sendto, socket, ioctlsocket, gethostbyname, gethostbyname_r, getaddrinfo, freeaddrinfo. These functions exist in IDF, but only as static inline functions. They need to be exported functions to be found by the linker.
  It can be done pretty easily, adding a C file with the following pattern:

  // defines an alias symbol:
  int accept_impl(int socket, struct sockaddr *restrict address, socklen_t *restrict address_len) \
       __attribute__((alias("accept"));
  
  // calls the "static inline function"
  int accept_impl(int socket, struct sockaddr *restrict address, socklen_t *restrict address_len) {
      return accept(socket, address, address_len);
  }

  (repeat for each function that is static inline and needs to be exported)
• Define getcwd. Can simply return "/".
• Define chdir. Can return -1 and set errno to ENOSYS.
• Define sysconf and _SC_PAGESIZE. Can be a very simple implementation, returning constant value.
• Process-related functions (kill, signal, exec, wait, spawn, etc) — no meaningful implementation possible, but each can return -1 and set errno to ENOSYS.
• nanosleep — can be a wrapper around usleep

More complex changes

• duplication of file descriptors — needs support in vfs component, not sure how complex this would be, and the semantics required by libstd.
• PTHREAD_COND_INITIALIZER definition in IDF is non-standard:

  // The newlib definitions in libc for pthread_mutex_t and PTHREAD_MUTEX_INITIALIZER
  // are way off compared to what we have in ESP-IDF:
  // 1) ESP-IDF's pthread_mutex_t is only 4 bytes and not 40+ (because it is in fact a pointer to dynamically allocated structure)
  // 2) While the above is just a waste of space, ESP-IDF PTHREAD_MUTEX_INITIALIZER is the real issue:
  //    in libc it is defined as a series of 0s, while in ESP-IDF it is equal to 0xffffffff
  

  This might be harder to change. I don't have a concrete idea about how to solve this, yet.

No changes

These changes in the current patch set do not seem to be necessary, need to find why they were needed.

• getrandom — it should be already supported by ESP-IDF.

---

igrr:
@ivmarkov here is the summary after looking at the std-related patches. Most things can be solved in IDF pretty easily.

There are probably other things in the patchset that i haven't noticed yet, please feel free to add more items.

---

ivmarkov:
Hi @igrr,

Thanks for all the feedback so far.

Before commenting on details, let me state something generic, which is very important:
Using an API (function) implemented in ESP-IDF from Rust STD is really a two-step process:

1. The function should (obviously) be available in ESP-IDF
2. The function should be declared by the libc crate

The reason why we need (2) is because currently the whole Rust STD support is based around consuming "extern C" API definitions available in the libc crate from above, which are manually defined and maintained for the various libC libraries (msvcrt, glibc, newlib, musl etc.).
Currently, Rust STD does not know about our esp-idf-sys crate and therefore about all the Bindgen bindings generated by it. Letting STD know about esp-idf-sys might increase the STD patch size and its complexity, and decrease the chances of it being accepted so far.

Therefore, I think we should stick to consuming the libc crate instead of our own autogenerated bindings in the context of Rust STD (files, threads and networking/sockets).
That would imply that we might need to file patches against libc (the newlib sub-module specifically), so that it properly reflects the status quo on ESP-IDF.

That work has already started. For example, @reitermarkus, who started the STD-on-ESP32 effort originally was able to push these changes to the libc crate, which were key for having the initial ESP32 STD support for Rust.
I think we should just continue this for riscv, as well as for all "deviations" ESP-IDF has from the standard newlib - particularly in the pthread area.

Of course not everything needs to be in the libc crate so as to be usable from Rust STD. You can always put an "inline" "extern C" definition, as I did e.g. here. It is just that these should be a minority, I feel.

One more thing: I'm not implying that there will be a huge amount of changes to the libc crate. Just that there will be some, and whenever e.g. there is an API in ESP-IDF (like posix_memalign) that does not necessarily means that it is exposed in libc/newlib.
As for the types of changes, see the riscv support here. Currently, a bit of a hack as it delegates to the "xtensa" support, even though I don't expect these to be different.

---

igrr:

• Regarding your assessment of the "easy" changes: I agree completely. Before starting with those, I'm wondering, perhaps we should approach the Rust libstd maintainers with the current patchset, to get early feedback / directions. Shall I do this, or will Espressif drive it?
• Regarding the "more complex ones":
  • FDs duplication, sematics: aren't they just expecting the POSIX semantics, whatever it is?
  • pthread*_t sizes being incorrect, and the PTHREAD_MUTEX_INITIALIZER of ESP-IDF not having the value defined in the libc crate for newlib - that might be something we have to push for fixing in the libc crate itself

---

igrr:
@ivmarkov Thanks for the explanation, I didn't know about newlib-specific things present in libc create, it makes a lot more sense now. I would appreciate if you could start the discussion with libstd maintainers regarding this existing patch set.

[ivmarkov]

@igrr, FYI:

I have been reflecting a bit on the current set of STD patches and looking into the libc Rust crate, and I have come up with updated patches, where the changeset is distributed a bit differently across (a) Rust libStd, (b) the libc crate and (c) the ESP-IDF C code itself.

Original plan:

• Minimal (as in 2-3 lines) changes to the libc crate
• The bulk of changes into the Rust libStd (itself) <-- biggest risk to address
• Initially zero, but over time maybe a set of non-trivial changes to ESP-IDF C code, to remove the lwip_ prefix and somehow (how?) fix the pthread constants and type sizes deviations from whatever is defined in the libc crate.

New plan (and patch set):

Idea is:

• Hide the ESP-IDF specifics of the newlib C port (lwip_, pthread) in the libc crate (seems everybody is doing similar stuff in libc);
• Less changes to libStd itself as a result
• Initially, zero changes to ESP-IDF. Might revisit in future, but the ESP-IDF changes will be for the "easy stuff" - stubbed functions. Pthread specifics and lwip_ prefix is now moved to the libc crate). Possible "hard" stuff in need to be implemented in ESP IDF itself remains the file descriptors duplication logic.

New patchset:

• libStd
• libc

Biggest remaining risks:

this
and
this.

[Urgau]

Just a random guy how saw the blog post Rust on Espressif chips on reddit.

I just wanted to say the Target Tier Policy of Rust state:

Tier 3 targets should attempt to implement as much of the standard libraries as possible and appropriate (core for most targets, alloc for targets that can support dynamic memory allocation, std for targets with an operating system or equivalent layer of system-provided functionality), but may leave some code unimplemented (either unavailable or stubbed out as appropriate), whether because the target makes it impossible to implement or challenging to implement.

It has been stated before by Rust projects guys (I don't remember where) that the unsupported shims (always returning an error if unsupported) has been a really bad idea and that new targets will probably don't want to do that and simply don't provide the functions.

I would suggest to simply don't provide the methods chdir, getcwd, ... and to simply leave out the process API and any other API that will never be supported on your target.

[ivmarkov]

Thanks for the feedback!

Unfortunately not providing the above-mentioned functionality is not that simple as that would mean polluting libStd code outside of sys/unix with #[cfg(target_os = "espidf")] #ifdefs. For example, to switch-off the process API, I might need to conditionally exclude this module and this module for the ESP-IDF.

Ditto for chdir and getcwd, where the situation might be even hairier, as these are individual methods.

Also, this guy (used by the wasi and wasm) seems to keep growing and growing, and I think the wasi target is not exactly old-ish.

Providing "not implemented" stubs also has the benefit that you have higher chances then in compiling off-the-shelf Rust crates (me running Rocket on the ESP-IDF in the past comes to mind as an example of a crate that does not compile if I do not stub out at least some of this stuff). And then you at least have some chances to be able to force the crate to not hit the not-implemented code-paths by configuration etc.

But - you know, submission of the patches is kind of imminent now, so if Rust maintainers object, removing functionality would be the way forward, whether I like it or not. :)

[ivmarkov]

@MabezDev @igrr @georgik

PRs submitted upstream:
libc
Rust LibStd

[Urgau]

Providing "not implemented" stubs also has the benefit that you have higher chances then in compiling off-the-shelf Rust crates (me running Rocket on the ESP-IDF in the past comes to mind as an example of a crate that does not compile if I do not stub out at least some of this stuff). And then you at least have some chances to be able to force the crate to not hit the not-implemented code-paths by configuration etc.

I agree that appreciable, but that's defeat the "if that's compile that's works" that so many people in the Rust community really appreciate. These kind of "not implemented" stubs are the opposite of that.

But - you know, submission of the patches is kind of imminent now, so if Rust maintainers object, removing functionality would be the way forward, whether I like it or not. :)

No problem that was just a suggestion from a random guy.

PRs submitted upstream:
libc
Rust LibStd

👍

[Urgau]

@ivmarkov One remark, I think that you should include the target tier policy with the part checked/explain for your target.
See this post from the BPF target support PR as an exemple.

EDIT: You also miss to update the src/doc/rustc/src/platform-support.md file with your new target.

[ivmarkov]

@Urgau Your comments & suggestions are really appreciated - please keep sending those.

• Thanks for the eagle eye on the platform-support.md file - I'll update it shortly
• I'll also update my PR post as you suggested - by "answering" / "confirming" the Tier 3 questionnaire & statements. I was afraid that I'm kind of too verbose and ceremonial already, but ... I guess it doesn't hurt.

[ivmarkov]

CI just complained about those targets missing from platform-support.md. You have a point.

[Urgau]

@Urgau Your comments & suggestions are really appreciated - please keep sending those.

❤️

By talking about suggestion I have one more. You stated:

Anything added to the Rust repository must be under the standard Rust license (MIT OR Apache-2.0).

MIT

I think that you need to agree to both of them because the Rust project is dual licensed against MIT and Apache-2.0, this dual licensing impose contributors to agree on both licenses when doing a pull request but non-contributor (ie user) can choose which license they want: MIT or Apache-2.0.

[ivmarkov]

@Urgau Fixed :)

[MabezDev]

With the libc & esp-idf std patches upstreamed into Rust, and the esp-idf patches tracked here and here, can we close this now?

[ivmarkov]

Yes.