Skip to content

Fix c_char on various no-std and tier 3 targets#131319

Closed
taiki-e wants to merge 1 commit intorust-lang:masterfrom
taiki-e:c-char
Closed

Fix c_char on various no-std and tier 3 targets#131319
taiki-e wants to merge 1 commit intorust-lang:masterfrom
taiki-e:c-char

Conversation

@taiki-e
Copy link
Member

@taiki-e taiki-e commented Oct 6, 2024 •
edited
Loading

Fixes #129945

there are many target_os = "none" target and tier3 targets that do not match.

  • As for the target_os = "none" target, most of them probably do not match with Clang's default.
    • I guess this is because c_char was originally defined in std and this mismatch was missed when it was moved to core.
    • I don't think many people use c_char directly on these targets, but this includes a lot of tier 2 targets, so changing this to match Clang could have a not small impact on embedded ecosystem.
  • All other targets seem to be tier 3, so I think changing them to match Clang's default should be no problem.
    • However, note that Clang's default may be wrong for minor architectures such as C-SKY. (see the reference below)

On MSP430 (tier 3), both Clang and rustc don't match with the ABI's default char type...

This PR makes c_char match with the Clang's default, except when Clang does not match the ABI's default.

TODO:

r? @ghost

@rustbot label -S-waiting-on-review, +S-waiting-on-author

@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 Oct 6, 2024
@taiki-e taiki-e force-pushed the c-char branch 2 times, most recently from 38245fc to f587fe3 Compare October 7, 2024 15:44
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2024
@workingjubilee workingjubilee added the A-ABI Area: Concerning the application binary interface (ABI) label Oct 21, 2024
kadiwa4
kadiwa4 reviewed Oct 27, 2024
View reviewed changes
library/core/src/ffi/mod.rs
// option is used."
// https://learn.microsoft.com/en-us/cpp/cpp/fundamental-types-cpp?view=msvc-170#character-types)
if #[cfg(all(
not(any(target_vendor = "apple", windows)),
Copy link
Contributor

@kadiwa4 kadiwa4 Oct 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
not(any(target_vendor = "apple", windows)),
not(target_vendor = "apple"),
not(windows),

would be easier to understand IMO

Copy link
Member Author

@taiki-e taiki-e Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for readability in case there are more “exception targets”.

See rust-lang/libc#4198 (comment) for more.

@tgross35 tgross35 mentioned this pull request Nov 15, 2024
Merged
arichardson added a commit to arichardson/rust that referenced this pull request Dec 10, 2024
@arichardson @taiki-e
Add references to the specific ABI documents
dd3e98c 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
@kadiwa4
Copy link
Contributor

kadiwa4 commented Dec 11, 2024 •
edited
Loading

This is being superseded by #132975 now

@bors bors closed this in fe516ef Dec 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2024
@rust-timer
Unrolled build for rust-lang#132975
e8ae496 
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
@taiki-e taiki-e deleted the c-char branch December 12, 2024 01:54
This was referenced Dec 17, 2024
Merged
Merged
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Feb 20, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
9084234 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Feb 20, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
d08ffaa 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to carolynzech/rust that referenced this pull request Feb 20, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
0652319 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Feb 20, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
6a874da 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Feb 21, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
989d216 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Feb 21, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
92a5ae2 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Feb 22, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
e82cda4 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to carolynzech/rust that referenced this pull request Feb 22, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
ffb9c47 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Feb 22, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
6967737 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Feb 22, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
ef04404 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Mar 3, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
face2ac 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to carolynzech/rust that referenced this pull request Mar 4, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
fe9c4b2 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to carolynzech/rust that referenced this pull request Mar 4, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
3a475cf 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Mar 4, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
87adc36 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 6, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
6c26c1f 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to thanhnguyen-aws/verify-rust-std that referenced this pull request Mar 6, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
8fb1a68 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
@arichardson @taiki-e
Add references to the specific ABI documents
cd585e8 
Expcept for L4RE and Xtensa these were obtained from rust-lang#131319

I could not find an open link to the Xtensa documentation, but the
signedness was confirmed by on of the Xtensa developers in
llvm/llvm-project#115967 (comment)

Co-authored-by: Taiki Endo <te316e89@gmail.com>
github-actions bot pushed a commit to tautschnig/verify-rust-std that referenced this pull request Mar 11, 2025
@matthiaskrgr
Rollup merge of rust-lang#132975 - arichardson:ffi-c-char, r=tgross35
c11bf59 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kadiwa4 kadiwa4 kadiwa4 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ABI Area: Concerning the application binary interface (ABI) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

c_char signedness doesn't match with Clang's default on various no-std and tier 3 targets

4 participants

@taiki-e @kadiwa4 @workingjubilee @rustbot