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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kadiwa4 kadiwa4 kadiwa4 left review comments

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