Clang's isSignedCharDefault() is incorrect for multiple architectures

[arichardson]

While debugging a signed/unsigned char issue I started looking at the list of triples that use unsigned chars by default and it appears the logic in Clang does not match the ABI documents for some architectures. I noticed some issues while comparing isSignedCharDefault() with the Rust list of systems with unsigned char: https://github.com/rust-lang/rust/blob/6503543d11583d1686d4989847b2afbec8d9fdba/library/core/src/ffi/mod.rs#L92

So far it appears s390x, csky, xtensa, msp430 are missing from isSignedCharDefault(), but there might be other omissions.

• csky: https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf says CSKY should have unsigned char by default
• Misread the clang source, is already correct s390x: https://github.com/IBM/s390x-abi states the range of char is 0 … 2^n − 1, i.e. it uses unsigned char
• msp430: https://www.ti.com/lit/pdf/slaa534 states the char type is unsigned by default, GCC agrees
• xtensa: GCC says char is unsigned, not sure where the ABI document is.

I don't see a CSKY GCC on godbolt, but https://godbolt.org/z/qaaW18znY confirms that the other architectures diverge from GCC

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: Alexander Richardson (arichardson)

While debugging a signed/unsigned char issue I started looking at the list of triples that use unsigned chars by default and it appears the logic in Clang does not match the ABI documents for some architectures. I noticed some issues while isSignedCharDefault() with the Rust list of systems with unsigned char: https://github.com/rust-lang/rust/blob/6503543d11583d1686d4989847b2afbec8d9fdba/library/core/src/ffi/mod.rs#L92

So far it appears, s390x and csky, xtensa and msp430 are missing from isSignedCharDefault(), but there might be other omissions.

• csky: https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf says CSKY should have unsigned char by default
• s390x: https://github.com/IBM/s390x-abi states the range of char is 0 … 2^n − 1, i.e. it uses unsigned char
• msp430: https://www.ti.com/lit/pdf/slaa534 states the char type is unsigned by default, GCC agrees
• xtensa: GCC says char is unsigned, not sure where the ABI document is.

I don't see a CSKY GCC on godbolt, but https://godbolt.org/z/qaaW18znY confirms that the other architectures diverge from GCC

[shafik]

CC @erichkeane @AaronBallman

[AaronBallman]

Something to consider as we fix these: we may need to provide ABI tags so users can still get the old (incorrect) ABI as needed.

[arichardson]

Do we really need a compatibility fallback here? The sign of char rarely matters for code generation and I believe none of the affected targets are "tier 1"? Affected projects can always use -f(no)-signed-char.

[AaronBallman]

Do we really need a compatibility fallback here?

Because this impacts the answer you get back from type traits like is_signed_v, and those tend to get used when specializing templates, the ABI impact can be surprisingly wide.

The sign of char rarely matters for code generation and I believe none of the affected targets are "tier 1"? Affected projects can always use -f(no)-signed-char.

That's a good point I hadn't considered, but it seems somewhat unclean to me. That gets us to "to get the Clang 19 ABI, pass -fclang-abi-compat=19 but you may also need to set -f(no)-signed-char as well" which isn't great.

[arichardson]

The template specialization is a good point, I had not considered that.

I think the main question is whether the affected targets have a real binary compatibility story or tend to be more of an embedded "recompile the whole system" target.

• MSP430 is a microcontroller, so I'd imagine limited binary compatibility concerns.
• I don't know anything about CSKY but it is 32-bit embedded with Linux support, so that may have stronger requirements.
• I also don't know much about XTensa, but it also appears to be used for embedded architectures.

We could forward args from RenderCharacterOptions to isSignedCharDefault add a check for OPT_fclang_abi_compat_EQ inside this function, but that would require some duplication unless we move this logic from the Driver to actual per-target LangOptions overrides.

Arguably the latter would be cleaner than handling this in the Driver (and there is already a FIXME). This would allow test/Preprocessor to check for the value of __CHAR_UNSIGNED__ instead of writing Driver tests as I did in #115967 and #115964. But I don't think I will be able find the time to do this larger refactoring any time soon.

[AaronBallman]

I think the main question is whether the affected targets have a real binary compatibility story or tend to be more of an embedded "recompile the whole system" target.

* MSP430 is a microcontroller, so I'd imagine limited binary compatibility concerns.

* I don't know anything about CSKY but it is 32-bit embedded with Linux support, so that may have  stronger requirements.

* I also don't know much about XTensa, but it also appears to be used for embedded architectures.

Yeah, my hope is always that people will be able to recompile everything at once, but my experience has been that there's a surprising number of situations where that's not plausible. My bigger concern is more that we tell users to use -fclang-abi-compat=X to get ABI compatibility with version X of Clang; I think it's useful for there to be one flag to regain ABI compatibility instead of having to combine flags to do so.

We could forward args from RenderCharacterOptions to isSignedCharDefault add a check for OPT_fclang_abi_compat_EQ inside this function, but that would require some duplication unless we move this logic from the Driver to actual per-target LangOptions overrides.

Arguably the latter would be cleaner than handling this in the Driver (and there is already a FIXME). This would allow test/Preprocessor to check for the value of __CHAR_UNSIGNED__ instead of writing Driver tests as I did in #115967 and #115964. But I don't think I will be able find the time to do this larger refactoring any time soon.

Yeah, I think handling this on a per-target basis would be cleaner than handling this in the driver. However, I don't think that larger refactoring should block fixing the behavior of whether char is signed or not.

Do we document ABI stability guarantees for any of the target tiers? e.g., can we skip ABI tags because these targets are not tier 1 and we document somewhere that they're not ABI stable as a result?

[llvmbot]

@llvm/issue-subscribers-clang-driver

Author: Alexander Richardson (arichardson)

While debugging a signed/unsigned char issue I started looking at the list of triples that use unsigned chars by default and it appears the logic in Clang does not match the ABI documents for some architectures. I noticed some issues while comparing isSignedCharDefault() with the Rust list of systems with unsigned char: https://github.com/rust-lang/rust/blob/6503543d11583d1686d4989847b2afbec8d9fdba/library/core/src/ffi/mod.rs#L92

So far it appears s390x, csky, xtensa, msp430 are missing from isSignedCharDefault(), but there might be other omissions.

• csky: https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf says CSKY should have unsigned char by default
• Misread the clang source, is already correct s390x: https://github.com/IBM/s390x-abi states the range of char is 0 … 2^n − 1, i.e. it uses unsigned char
• msp430: https://www.ti.com/lit/pdf/slaa534 states the char type is unsigned by default, GCC agrees
• xtensa: GCC says char is unsigned, not sure where the ABI document is.

I don't see a CSKY GCC on godbolt, but https://godbolt.org/z/qaaW18znY confirms that the other architectures diverge from GCC

[arichardson]

MSP430 has been fixed (so github autoclosed this) but others may need a compatibility storage.