Clang's isSignedCharDefault() is incorrect for multiple architectures #115957
Comments
arichardson
added
clang
|
@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.
I don't see a CSKY GCC on godbolt, but https://godbolt.org/z/qaaW18znY confirms that the other architectures diverge from GCC |
|
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. |
|
Do we really need a compatibility fallback here? The sign of |
Because this impacts the answer you get back from type traits like
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 |
|
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.
We could forward args from 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 |
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).
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
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 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? |
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).
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).
EugeneZelenko
added
clang:driver
|
@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
I don't see a CSKY GCC on godbolt, but https://godbolt.org/z/qaaW18znY confirms that the other architectures diverge from GCC |
|
MSP430 has been fixed (so github autoclosed this) but others may need a compatibility storage. |
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: rust-lang#129945
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: rust-lang#129945
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: rust-lang#129945
De-duplicate and improve definition of core::ffi::c_char 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 rust-lang#129945 Closes rust-lang#131319
Rollup merge of rust-lang#132975 - arichardson:ffi-c-char, r=tgross35 De-duplicate and improve definition of core::ffi::c_char 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 rust-lang#129945 Closes rust-lang#131319
This matches the ABI document found at /~https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf Partially addresses #115957 Reviewed By: zixuan-wu Pull Request: #115961
|
This should all be fixed now. /cherry-pick d204724 |
Error: Command failed due to missing milestone. |
|
/cherry-pick d204724 |
|
/pull-request #126436 |
This matches the ABI document found at /~https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf Partially addresses llvm/llvm-project#115957 Reviewed By: zixuan-wu Pull Request: llvm/llvm-project#115961
This matches the ABI document found at /~https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf Partially addresses llvm#115957 Reviewed By: zixuan-wu Pull Request: llvm#115961
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: rust-lang#129945
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: rust-lang#129945
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: rust-lang#129945
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: rust-lang#129945
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: rust-lang#129945
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: rust-lang#129945
This matches the ABI document found at /~https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf Partially addresses llvm/llvm-project#115957 Reviewed By: zixuan-wu Pull Request: llvm/llvm-project#115961 (cherry picked from commit d204724)
This matches the ABI document found at /~https://github.com/c-sky/csky-doc/blob/master/C-SKY_V2_CPU_Applications_Binary_Interface_Standards_Manual.pdf Partially addresses llvm/llvm-project#115957 Reviewed By: zixuan-wu Pull Request: llvm/llvm-project#115961 (cherry picked from commit d204724)
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: rust-lang#129945
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: rust-lang#129945
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: rust-lang#129945
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: rust-lang#129945
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.s390x: /~https://github.com/IBM/s390x-abi states the range ofcharis0 … 2^n − 1, i.e. it usesunsigned charthe char type is unsigned by default, GCC agreesI don't see a CSKY GCC on godbolt, but https://godbolt.org/z/qaaW18znY confirms that the other architectures diverge from GCC
The text was updated successfully, but these errors were encountered: