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

Revert vita's c_char back to i8 #136667

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pheki
Copy link
Contributor

@pheki pheki commented Feb 7, 2025

Description

Hi!

#132975 changed the definition of c_char from i8 to u8 for most ARM targets. While that would usually be correct, VITASDK uses signed chars by default. The Clang definitions are incorrect because Clang is not (yet?) supported by the vita commmunity / VITADSK, On the Rust side, the pre-compiled libraries the user can link to are all compiled using vita's gcc and we set TARGET_CC and TARGET_CXX in cargo vita for build scripts using cc.

I'm creating it as a draft PR so that we can discuss it and possibly get it approved here, but wait to merge the libc side and get a libc version first, as having the definitions out of sync breaks std. As a nightly-only target it can be confusing/frustrating for new users when the latest nightly, which is the default, is broken.

@rustbot
Copy link
Collaborator

rustbot commented Feb 7, 2025

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 7, 2025
@workingjubilee
Copy link
Member

is there a bug open against clang?

@workingjubilee
Copy link
Member

( or should someone else be fixing the clang-side issue? )

@pheki
Copy link
Contributor Author

pheki commented Feb 8, 2025

is there a bug open against clang?

I don't think so. In my understanding it doesn't have support for the Vita specifically, nor have I seen evidence of people using Clang for the Vita. I investigated a bit on what would it take to fix it there, AFAIK this would be the best way:

  1. Adding support for the Vita as a separate OSType. I have no idea if they have a policy for new targets.
  2. Make isSignedCharDefault return true for the Vita.

I'm not sure if the changes would need to be tested. To be able to test these changes, someone would need to figure out:

  1. Linker scripts to generate the ELF correctly
  2. Other compiler/linker options (e.g. for custom sysroot)
  3. The usual process for going from an ELF into a VPK (installable package).

Overall it doesn't seem that complicated and it would be great if someone figured it out, but at this moment I'm personally more interested in fixing the Rust toolchain (which doesn't use Clang, only LLVM). To be honest, I feel a bit awkward creating a bug report for a platform that's not yet supported 😅

@ibraheemdev
Copy link
Member

I'm not the best person to review this. r? libs

@rustbot rustbot assigned cuviper and unassigned ibraheemdev Feb 11, 2025
@cuviper
Copy link
Member

cuviper commented Feb 11, 2025

@pheki, I see that you are one of the target maintainers, so let's check with the others:

@nikarh and @zetanumbers, any comment?

@nikarh
Copy link
Contributor

nikarh commented Feb 11, 2025

@pheki, I see that you are one of the target maintainers, so let's check with the others:

@nikarh and @zetanumbers, any comment?

Agree with @pheki.
VitaSDK uses signed char by default, and vita target requires VitaSDK toolchain.
Considering that, the easiest solution for us would be changing char to signed in rust-lang/rust and rust-lang/libc for this target.

@zetanumbers
Copy link
Contributor

zetanumbers commented Feb 12, 2025

pheki, I see that you are one of the target maintainers, so let's check with the others:

nikarh and zetanumbers, any comment?

LGTM. I remember signed by default char being intentional. VITASDK solely uses (old patched version of) gcc, and I am not aware of any existing llvm toolchain for vita.

@cuviper
Copy link
Member

cuviper commented Feb 13, 2025

Thanks everyone! I'm marking this blocked until the libc update is ready to go with it.

@rustbot blocked

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants