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

Update ESP-IDF c_char type #4195

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

SergioGasquez
Copy link
Contributor

@SergioGasquez SergioGasquez commented Dec 13, 2024

Description

Updates the c_chart type for ESP-IDF after rust-lang/rust#132975

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI
    • Still not tested, hence the PR is still in draft

@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2024

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

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.

Any reason this is marked as a draft?

@SergioGasquez
Copy link
Contributor Author

@ivmarkov wanted to do a small test, that's why I marked it as draft until we confirm that it works, but if you think the change is small enough I can mark it as ready

@tgross35
Copy link
Contributor

Happy to have tests! There probably won't be another release until next week anyway.

@ivmarkov
Copy link
Contributor

Happy to have tests! There probably won't be another release until next week anyway.

Tests passed successfully.
TL;DR: libc's c_char and core::ffi::c_char should both be u8, for all *-espidf targets (i.e. both riscv as well as xtensa).

@SergioGasquez if you could "un-draft" the PR?

Long explanation:

  • I checked the Espressif's GCC toolchains (both xtensa and riscv) and both report the char type as unsigned by default (limits.h's CHAR_MAX is 255).
  • I also checked Espressif's clang toolchains and there:
    • clang with riscv also agrees that CHAR_MAX is 255 (i.e. unsigned)
    • clang with xtensa still reports char as signed, but this is only becausde the Espressif's LLVM+clang xtensa fork is not yet updated with the upstream clang xtensa fix which marks char as unsigned. There is an open MR to fix the fork to treat char as unsigned (just like the upstream code), which will be merged soon

@SergioGasquez SergioGasquez marked this pull request as ready for review December 14, 2024 12:54
@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Dec 14, 2024
@tgross35
Copy link
Contributor

Thanks for confirming!

@tgross35 tgross35 added this pull request to the merge queue Dec 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 14, 2024
@ivmarkov
Copy link
Contributor

@tgross35 somehow this PR did fall out from the merge queue. Is it something we need to address? Weird.

@tgross35 tgross35 added this pull request to the merge queue Dec 16, 2024
@tgross35
Copy link
Contributor

Thanks for the reminder, I tried adding it back. We just have some tests that are a bit flaky.

Merged via the queue into rust-lang:main with commit 39a6799 Dec 16, 2024
45 checks passed
@SergioGasquez SergioGasquez deleted the feat/c_char branch December 17, 2024 09:04
@ivmarkov
Copy link
Contributor

@tgross35 Sorry to bother - but I think we need this in the 0.2 branch as well.

@tgross35
Copy link
Contributor

Indeed, it has the label so it will be in the next release. I was originally planning to do a release next week - but I guess ideally you need it as soon as possible?

Assuming so, I'll try to get one out within the next couple days instead.

@ivmarkov
Copy link
Contributor

Indeed, it has the label so it will be in the next release. I was originally planning to do a release next week - but I guess ideally you need it as soon as possible?

Assuming so, I'll try to get one out within the next couple days instead.

Yes, ASAP would be deeply appreciated as we are broken since... some time. :-)

Thank you so much!

@taiki-e taiki-e mentioned this pull request Dec 17, 2024
3 tasks
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Dec 18, 2024
(backport <rust-lang#4195>)
(cherry picked from commit c66faeb)
@tgross35 tgross35 mentioned this pull request Dec 18, 2024
@tgross35 tgross35 removed the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Dec 18, 2024
@tgross35 tgross35 added the stable-applied This PR has been cherry-picked to libc's stable release branch label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-newlib O-unix 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.

4 participants