De-duplicate and improve definition of core::ffi::c_char

[arichardson]

Instead of having a list of unsigned char targets for each OS, follow the logic Clang uses and instead set the value based on architecture with a special case for Darwin and Windows operating systems. This makes it easier to support new operating systems targeting Arm/AArch64 without having to modify this config statement for each new OS. The new list does not quite match Clang since I noticed a few bugs in the Clang implementation (llvm/llvm-project#115957).

Fixes #129945
Closes #131319

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (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

[arichardson]

r? @maurer @joshtriplett

[rustbot]

Failed to set assignee to maurer: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[beetrees]

I think r? @joshtriplett should work.

[maurer]

Some OSes are no longer conditioned on, for example, os=nto arch=aarch64 or os=l4re, arch=x86_64 are missing.

I didn't manually verify everything, but at least l4re and x86_64 just got flipped from u8, which it appears to have been manually set to, to i8.

Can we get a summary of what changed in terms of supported target triples? I would expect disagreements with clang to be rare, but this patch looks like it accidentally changes obscure targets at a minimum. It might also be clearer what changed if we didn't have the 4-branch if statement for 2 results.

[maurer]

I'm pretty sure this still changes l4re-x86_64 from unsigned to signed.

[beetrees]

As Apple and Windows targets always use signed char, you could simplify the cfg statement further to cfg(all(not(any(windows, target_vendor = "apple")), any(/* target_arch/target_os that use unsigned char */)))

[arichardson]

I'm pretty sure this still changes l4re-x86_64 from unsigned to signed.

I'm trying to figure out if that value is intentional or not. It dates back to 2cf0a4a, but I can't see anything in L4RE that changes the default - maybe this was actually indented to set it for aarch64?

[arichardson]

Looks like l4re should be setting this for all architectures, I spotted -funsigned-char in https://github.com/kernkonzept/mk/blob/926afa93e32e64dbdb33cf9ae724924ee1fb16e0/tool/kconfig/Makefile#L550, so that suggests it's the case for all targets. Will update to handle that.

[joshtriplett]

r? libs-api

[tgross35]

See also an open issue about this #129945 and an open PR that this would supersede #131319 (cc @taiki-e)

I also wrote an issue requesting a test of our types against Clang at some point #133058.

[arichardson]

I have validated the new c_char definitions against my build of clang that has fixes for MSP430, Xtensa (although printing the builtin defines fails with error: unknown target triple 'xtensa-unknown-none-elf'), and CSKY (which fails with version 'abiv2' in target triple 'csky-unknown-linux-gnuabiv2' is invalid).

Unlike #131319 this PR does not have links to all the ABI docs, but it does have a correct definition for l4re (which clang does not support and treats as unknown OS).

[taiki-e]

As for L4Re:
I think L4Re code you are linking is config when building kernel. IIUC, userland uses signed for x86_64 and unsigned for aarch64 by default (i.e., same as ELF ABI's default).

$ ./toolchain-l4re-x86_64-gcc-14/bin/x86_64-l4re-gcc -E -dM -x c /dev/null | grep -F __CHAR_
#define __CHAR_BIT__ 8

$ ./toolchain-l4re-arm64-gcc-14/bin/aarch64-l4re-gcc -E -dM -x c /dev/null | grep -F __CHAR_
#define __CHAR_BIT__ 8
#define __CHAR_UNSIGNED__ 1

({x86_64,aarch64}-l4re-gcc are from https://l4re.org/download/snapshots/toolchain/.)

---

As for Xtensa:
Section 2.17.1 "Data Types and Alignment" of Xtensa LX Microprocessor Overview handbook https://loboris.eu/ESP32/Xtensa_lx%20Overview%20handbook.pdf says "char type is unsigned by default".

However, it appears that Xtensa has multiple ABIs. (Section 1.4 "Calling convention" of Overview of Xtensa ISA https://dl.espressif.com/github_assets/espressif/xtensa-isa-doc/releases/download/latest/Xtensa.pdf)
(I have not checked whether the defaults are consistent among those ABIs or not -- this is why my PR has done nothing about Xtensa.)

[arichardson]

Rebased and fixed the l4re issue.

As for L4Re: I think L4Re code you are linking is config when building kernel. IIUC, userland uses signed for x86_64 and unsigned for aarch64 by default (i.e., same as ELF ABI's default).

$ ./toolchain-l4re-x86_64-gcc-14/bin/x86_64-l4re-gcc -E -dM -x c /dev/null | grep -F __CHAR_
#define __CHAR_BIT__ 8

$ ./toolchain-l4re-arm64-gcc-14/bin/aarch64-l4re-gcc -E -dM -x c /dev/null | grep -F __CHAR_
#define __CHAR_BIT__ 8
#define __CHAR_UNSIGNED__ 1

({x86_64,aarch64}-l4re-gcc are from https://l4re.org/download/snapshots/toolchain/.)

Thanks for checking this, I've updated the change to remove l4re from the list. I've done this as a separate commit in case this needs to be reverted in the future.

As for Xtensa: Section 2.17.1 "Data Types and Alignment" of Xtensa LX Microprocessor Overview handbook https://loboris.eu/ESP32/Xtensa_lx%20Overview%20handbook.pdf says "char type is unsigned by default".

However, it appears that Xtensa has multiple ABIs. (Section 1.4 "Calling convention" of Overview of Xtensa ISA https://dl.espressif.com/github_assets/espressif/xtensa-isa-doc/releases/download/latest/Xtensa.pdf) (I have not checked whether the defaults are consistent among those ABIs or not -- this is why my PR has done nothing about Xtensa.)

Regarding Xtensa, my Clang change to make it unsigned by default was approved by Xtensa developers and has been merged now: llvm/llvm-project#115967.

[tgross35]

Pinging some target maintainers (checkbox = acked):

• Solid @kawadakk
• Hermit @stlankes @mkroening
• Illumos @jclulow @sunshowers
• Redox @jackpot51
• Trusty @randomPoison @ahomescu
• Hexagon @androm3da
• RISC-V @robin-randhawa-sifive @raw-bin
• RISC-V Android @chriswalles @maurer
• Xous @xobs
• ESP-IDF @ivmarkov @MabezDev
• Nuttx @no1wudi
• Thumb @Patryk27

If you were pinged here, this PR will change one of your target's definition of c_char to be in line with Clang's, most from i8 to u8. Not all targets change, this is pretty limited to Arm and RISC-V. The full list of changing targets is at #129945 and the list after being fixed is #132975 (comment), let us know if you know something to be incorrect here.

[jclulow]

* it primarily affects embedded targets (-unknown-) which are less likely to interact with C code.

I don't think that's actually what -unknown- means, FWIW. Looking at the platform support list I see quite a lot of targets that contain the string -unknown- and are not embedded systems.

[jclulow]

I have confirmed with the folks working on the illumos ARM port that char is unsigned there to match the prevailing winds, so this makes sense to us, thanks!

[randomPoison]

This is correct for Trusty as far as I know, we expect c_char to be u8 on both aarch64 and arm.

[arichardson]

aarch64-unknown-uefi

Interesting, it looks like this is probably a bug in the target definition, which means it uses the windows ABI in Clang:
llvm_target: "aarch64-unknown-windows".into(),

This should be "aarch64-unknown-uefi" which Clang understands

❯ ~/upstream-llvm-project/cmake-build-debug/bin/clang -E -dM -x c /dev/null -target aarch64-unknown-uefi | grep __CHAR_UN
#define __CHAR_UNSIGNED__ 1
❯ ~/upstream-llvm-project/cmake-build-debug/bin/clang -E -dM -x c /dev/null -target aarch64-unknown-windows | grep __CHAR_UN

[jackpot51]

This is fine for Redox OS.

[tgross35]

aarch64-unknown-uefi

Interesting, it looks like this is probably a bug in the target definition, which means it uses the windows ABI in Clang: llvm_target: "aarch64-unknown-windows".into(),

This should be "aarch64-unknown-uefi" which Clang understands

That is mildly amusing. Opened #134156 to fix it.

[tgross35]

With the libs-api approval I don't think there is any reason to hold off, especially considering it was mentioned during the meeting that this is better to have toward the beginning of a release cycle (we are reasonably early now, next branch is January 3rd for stable on February 20th).

@bors r+

Thanks everyone who took a look at this.

[bors]

📌 Commit dd3e98c has been approved by tgross35

It is now in the queue for this repository.

[arichardson]

aarch64-unknown-uefi

Interesting, it looks like this is probably a bug in the target definition, which means it uses the windows ABI in Clang: llvm_target: "aarch64-unknown-windows".into(),
This should be "aarch64-unknown-uefi" which Clang understands

That is mildly amusing. Opened #134156 to fix it.

Digging deeper it looks like this is only partially supported in Clang (x86 supports the triple, aarch64 triggers assertions). I do wonder if Clang's output here is wrong and Rust is correct - I imagine UEFI follows the Windows ABIs since it is COFF-based? But I haven't found a spec for the UEFI AArch64 ABI.

[androm3da]

Confirmed: for hexagon targets, c_char should be unsigned. ✅

Thanks for the fix.

[ivmarkov]

Pinging some target maintainers (checkbox = acked):

• Solid @kawadakk
• Hermit @stlankes @mkroening
• Illumos @jclulow @sunshowers
• Redox @jackpot51
• Trusty @randomPoison @ahomescu
• Hexagon @androm3da
• RISC-V @robin-randhawa-sifive @raw-bin
• RISC-V Android @chriswalles @maurer
• Xous @xobs
• ESP-IDF @ivmarkov @MabezDev
• Nuttx @no1wudi

If you were pinged here, this PR will change one of your target's definition of c_char to be in line with Clang's, most from i8 to u8. Not all targets change, this is pretty limited to Arm and RISC-V. The full list of changing targets is at #129945 and the list after being fixed is #132975 (comment), let us know if you know something to be incorrect here.

It looks like indeed c_char should be unsigned for both xtensa and riscv ESP-IDF targets. Yet - we are likely to have downstream breakages because of this change, as I noticed there are (a few) places where we have frivolously used c_char and i8 interchangeably, and obviously these places need to be fixed to only use core::ffi::c_char now.

With that said, I guess it is on us to fix this downstream, and we'll know the impact pretty soon, given the "bors +r" from a couple of hours ago.

[mkroening]

This aligns Hermit to its definitions in libc, so this is good from our side. Thanks! :)

[ivmarkov]

Hmm, looks like we need to fix then the ESP IDF c_char definition in libc, as it is i8 and this would now be different from the core::ffi::c_char definition.

I also wonder (excuse my ignorance) what this change means with regards to interop with C code compiled with GCC, as it seems GCC always treats char as a signed char, unless you tell it otherwise?

[tgross35]

Hmm, looks like we need to fix then the ESP IDF c_char definition in libc, as it is i8 and this would now be different from the core::ffi::c_char definition.

libc will need to be updated after this, most of the targets listed here probably have the same definition in libc.

I also wonder (excuse my ignorance) what this change means with regards to interop with C code compiled with GCC, as it seems GCC always treats char as a signed char, unless you tell it otherwise?

I think this answer might just be referring to x86, https://gcc.gnu.org/onlinedocs/gcc-4.2.2/gcc/C-Dialect-Options.html says:

Each kind of machine has a default for what char should be. It is either like unsigned char by default or like signed char by default.

And based on testing a few targets at https://godbolt.org/z/vxvY34W8b, at least the ARM and RISC-V targets seem to have CHAR_UNSIGNED defined.

AVR GCC actually does not have this defined for the thumb targets so we might need to double check those https://godbolt.org/z/rdroc9fe9 (@Patryk27).

It looks like indeed c_char should be unsigned for both xtensa and riscv ESP-IDF targets. Yet - we are likely to have downstream breakages because of this change, as I noticed there are (a few) places where we have frivolously used c_char and i8 interchangeably, and obviously these places need to be fixed to only use core::ffi::c_char now.

With that said, I guess it is on us to fix this downstream, and we'll know the impact pretty soon, given the "bors +r" from a couple of hours ago.

To be clear, we can make followup changes to specific targets as needed, or if the fallout is especially bad in particular areas.

[taiki-e]

@tgross35

AVR GCC actually does not have this defined for the thumb targets so we might need to double check those

thumb is thumb mode of 32-bit Arm (target_arch = "arm"). AVR GCC is for AVR (target_arch = "avr") and unrelated to thumb.

AVR's c_char is signed by default as mentioned in the references in #129945.

AVR
https://gcc.gnu.org/wiki/avr-gcc#Type_Layout says char is signed

Arm's c_char is not affected by whether it is in Arm or Thumb mode.