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

Fix ESP-IDF constants and structs #3920

Merged
merged 2 commits into from
Oct 13, 2024
Merged

Conversation

SergioGasquez
Copy link
Contributor

@SergioGasquez SergioGasquez commented Sep 13, 2024

This PR fixes some constants and structs which were wrong for ESP-IDF targets. I created an app that compares all the constants in src/unix/newlib and src/unix/newlib/espidf modules with the ones in ESP-IDF source code (accessed via esp_idf_svc::sys). Here is the app https://github.com/SergioGasquez/libc-checks, using libc@0.2.158 yields the following errors:

E (622) libc_checks: Mismatch detected for constant `SIGABRT`: `esp-idf` 6 | `libc` 1
E (632) libc_checks: Mismatch detected for constant `SIGFPE`: `esp-idf` 8 | `libc` 1
E (642) libc_checks: Mismatch detected for constant `SIGILL`: `esp-idf` 4 | `libc` 1
E (652) libc_checks: Mismatch detected for constant `SIGINT`: `esp-idf` 2 | `libc` 1
E (662) libc_checks: Mismatch detected for constant `SIGSEGV`: `esp-idf` 11 | `libc` 1
E (672) libc_checks: Mismatch detected for constant `SIGTERM`: `esp-idf` 15 | `libc` 1
E (672) libc_checks: Mismatch detected for constant `SIGQUIT`: `esp-idf` 3 | `libc` 1
E (682) libc_checks: Mismatch detected for constant `NSIG`: `esp-idf` 32 | `libc` 2
E (692) libc_checks: Mismatch detected for type `tcflag_t` size: `esp-idf` 2 | `libc` 4
E (702) libc_checks: Mismatch detected for type `tcflag_t` alignment: `esp-idf` 2 | `libc` 4
E (712) libc_checks: Mismatch detected for type `sigaction` size: `esp-idf` 12 | `libc` 72
E (722) libc_checks: Mismatch detected for type `termios` size: `esp-idf` 28 | `libc` 52
E (732) libc_checks: Mismatch detected for type `pthread_attr_t` alignment: `esp-idf` 4 | `libc` 1
E (742) libc_checks: Mismatch detected for constant `NCCS`: `esp-idf` 11 | `libc` 32
E (752) libc_checks: Mismatch detected for constant `O_CLOEXEC`: `esp-idf` 262144 | `libc` 524288
E (762) libc_checks: Mismatch detected for constant `PF_INET6`: `esp-idf` 10 | `libc` 23
E (762) libc_checks: Mismatch detected for constant `SOCK_CLOEXEC`: `esp-idf` 262144 | `libc` 524288
E (772) libc_checks: Mismatch detected for constant `TCP_NODELAY`: `esp-idf` 1 | `libc` 8193
E (782) libc_checks: Mismatch detected for constant `TCP_KEEPIDLE`: `esp-idf` 3 | `libc` 256
E (792) libc_checks: Mismatch detected for constant `TCP_KEEPINTVL`: `esp-idf` 4 | `libc` 512
E (802) libc_checks: Mismatch detected for constant `TCP_KEEPCNT`: `esp-idf` 5 | `libc` 1024
E (812) libc_checks: Mismatch detected for constant `IP_TOS`: `esp-idf` 1 | `libc` 3
E (822) libc_checks: Mismatch detected for constant `IP_TTL`: `esp-idf` 2 | `libc` 8
E (832) libc_checks: Mismatch detected for constant `IP_MULTICAST_IF`: `esp-idf` 6 | `libc` 9
E (842) libc_checks: Mismatch detected for constant `IP_MULTICAST_TTL`: `esp-idf` 5 | `libc` 10
E (842) libc_checks: Mismatch detected for constant `IP_MULTICAST_LOOP`: `esp-idf` 7 | `libc` 11
E (852) libc_checks: Mismatch detected for constant `IP_ADD_MEMBERSHIP`: `esp-idf` 3 | `libc` 11
E (862) libc_checks: Mismatch detected for constant `IP_DROP_MEMBERSHIP`: `esp-idf` 4 | `libc` 12
E (872) libc_checks: Mismatch detected for constant `IPV6_MULTICAST_IF`: `esp-idf` 768 | `libc` 9
E (882) libc_checks: Mismatch detected for constant `IPV6_MULTICAST_HOPS`: `esp-idf` 769 | `libc` 10
E (892) libc_checks: Mismatch detected for constant `IPV6_MULTICAST_LOOP`: `esp-idf` 770 | `libc` 11
E (902) libc_checks: Mismatch detected for constant `HOST_NOT_FOUND`: `esp-idf` 210 | `libc` 1
E (912) libc_checks: Mismatch detected for constant `NO_DATA`: `esp-idf` 211 | `libc` 2
E (922) libc_checks: Mismatch detected for constant `NO_RECOVERY`: `esp-idf` 212 | `libc` 3
E (932) libc_checks: Mismatch detected for constant `TRY_AGAIN`: `esp-idf` 213 | `libc` 4
E (942) libc_checks: Mismatch detected for constant `AI_NUMERICSERV`: `esp-idf` 8 | `libc` 0
E (952) libc_checks: Mismatch detected for constant `AI_ADDRCONFIG`: `esp-idf` 64 | `libc` 0
E (962) libc_checks: Mismatch detected for constant `NI_NUMERICSERV`: `esp-idf` 8 | `libc` 0
E (972) libc_checks: Mismatch detected for constant `NI_DGRAM`: `esp-idf` 16 | `libc` 0
E (972) libc_checks: Mismatch detected for constant `EAI_FAMILY`: `esp-idf` 204 | `libc` -303
E (982) libc_checks: Mismatch detected for constant `EAI_MEMORY`: `esp-idf` 203 | `libc` -304
E (992) libc_checks: Mismatch detected for constant `EAI_NONAME`: `esp-idf` 200 | `libc` -305
E (1002) libc_checks: Mismatch detected for constant `EAI_SOCKTYPE`: `esp-idf` 10 | `libc` -307

All those errors are fixed in this PR, if I point the app to use the branch of this PR, no errors appear.

cc and thanks to @ivmarkov!

closes #3774

@rustbot
Copy link
Collaborator

rustbot commented Sep 13, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@ivmarkov
Copy link
Contributor

@JohnTitor Just to mention that this PR is a continuation of #3345

What we did is to

  • Take the original C comparison code of @zetanumbers
  • Translate it to Rust so that we can directly use the libc types and constants
  • Extend it with more types
  • And then run the libc types against our "source of truth", which is the bindgen-generated Rust bindings of the ESP-IDF complete API - which - amongst other things - contains all newlib types with their proper alignment and size on ESP-IDF.

Obviously, we are running the comparison on the actual Espressif MCUs (or an emulation of those). And will likely continue doing so, in CI.

@ivmarkov
Copy link
Contributor

@JohnTitor Sorry for the ping. Wondering if there is anything else we need to do so as this PR to be considered in a good shape for merging.

@ivmarkov
Copy link
Contributor

ivmarkov commented Oct 6, 2024

?r @tgross35 Would you mind reviewing?

The PR should be relatively uncontroversial in that it affects newlib/ESP IDF only and fixes obvious bugs (wrong definitions).

@ivmarkov
Copy link
Contributor

@SergioGasquez Can you assign this to @tgross35 please?

@JohnTitor @tgross35 if both of you are busy, would you suggest a reviewer that can look into this?

@SergioGasquez
Copy link
Contributor Author

?r @tgross35

@SergioGasquez
Copy link
Contributor Author

?r @Amanieu

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Oct 13, 2024
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

The non-espif newlib headers look correctly unchanged, if your tests indicate that things are fixed for espif then that seems reasonable to me. Please link headers in this PR if they are available.

Note that this repo just has a somewhat slow turn time for a handful of reasons, I keep an eye on all PRs even if they aren't assigned to me. (And the review request syntax is r?, not ?r).

@tgross35 tgross35 added this pull request to the merge queue Oct 13, 2024
Merged via the queue into rust-lang:main with commit 5b554dd Oct 13, 2024
41 checks passed
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Oct 15, 2024
(backport <rust-lang#3920>)
(cherry picked from commit 133d9d0)
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Oct 15, 2024
(backport <rust-lang#3920>)
(cherry picked from commit 6f2b73a)
@tgross35 tgross35 mentioned this pull request Oct 15, 2024
@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Oct 15, 2024
@SergioGasquez SergioGasquez deleted the fix/esp-idf branch October 15, 2024 09:18
@jothan
Copy link

jothan commented Oct 22, 2024

@MabezDev would it be possible to bump the libc dependency of the std crate on the next compiler build ?

It would likely fix a lot of issues for a lot of people.

Thanks as always !

@SergioGasquez
Copy link
Contributor Author

Upstream already bumped the libc version, which was included in 1.84 nightly, so, worst case scenario we will have the changes in 2 releases, but maybe we could bump it for 1.83.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TCP_NODELAY has the wrong value on ESP32 (newlib / LWIP based)
6 participants