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

guarantee that char and u32 are ABI-compatible #118032

Merged
merged 1 commit into from
Dec 11, 2023
Merged

guarantee that char and u32 are ABI-compatible #118032

merged 1 commit into from
Dec 11, 2023

Conversation

RalfJung
Copy link
Member

In #116894 we added a guarantee that char has the same alignment as u32, but there is still one axis where these types could differ: function call ABI. So let's nail that down as well: in a function signature, char and u32 are completely equivalent.

This is a new stable guarantee, so it will need t-lang approval.

@rustbot
Copy link
Collaborator

rustbot commented Nov 18, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 18, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 18, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

@RalfJung RalfJung added T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 18, 2023
@RalfJung RalfJung mentioned this pull request Nov 18, 2023
Merged
@RalfJung
Copy link
Member Author

Cc @rust-lang/opsem

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 18, 2023
@scottmcm scottmcm removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 2023
@scottmcm
Copy link
Member

scottmcm commented Nov 22, 2023 •
edited
Loading

We discussed this in the T-Lang triage meeting today.

There was some discussion about whether it'd ever be reasonable to want to do something more optimal here -- maybe passing three chars in a 64-bit register, or using a 24-bit register on a chip that had such a thing.

But as I understood it, the takeaway was that those were considered too unusual to be worth doing, vs the simple rule that people are probably already expecting to be the case (given the size align match), so let's get confirmation of that:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 22, 2023 •
edited by pnkfelix
Loading

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 22, 2023
@scottmcm scottmcm added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 2023
@traviscross traviscross removed the I-lang-nominated The issue / PR has been nominated for discussion during a lang team meeting. label Nov 27, 2023
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 30, 2023
@rfcbot
Copy link

rfcbot commented Nov 30, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 30, 2023
@pnkfelix
Copy link
Member

Hey @rust-lang/project-exploit-mitigations , is it okay to guarantee that char and u32 are equivalent in the context of how function calling convention works in the ABI?

@rcvalle
Copy link
Member

rcvalle commented Nov 30, 2023

@pnkfelix thanks for pinging us! I've a few questions so I have a bit more context:

  1. I'm assuming that char and u32 will continue to be different types (i.e., can't be used interchangeably in Rust). Is that correct?
  2. Is this guarantee for extern "C" functions used with FFI only?
  3. Are we expecting that if we declare or define an extern "C" fn func(_: char) it'll be compatible with a C void func(uint32_t arg)?

@RalfJung
Copy link
Member Author

RalfJung commented Nov 30, 2023 •
edited
Loading

I'm assuming that char and u32 will continue to be different types (i.e., can't be used interchangeably in Rust). Is that correct?

Yes. char also still has the invariant that it must be a valid unicode codepoint.

Is this guarantee for extern "C" functions used with FFI only?

This is a guarantee for all our ABIs.

This is similar to how we e.g. already guarantee that NonZeroU32 and u32 are ABI-compatible.

Are we expecting that if we declare or define an extern "C" fn func(_: char) it'll be compatible with a C void func(uint32_t arg)?

Yes. I am not sure if we are currently officially documenting anywhere that u32 in Rust and uint32_t in C are ABI-compatible, but once that is established, it follows by transitivity that char in Rust and uint32_t in C are ABI-compatible.

@rcvalle
Copy link
Member

rcvalle commented Dec 1, 2023

Thank you for all the information, @RalfJung! Np, I think this should be added to the CFI integer normalization option then. Just let me know by when it needs to be done.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2023

Well this PR will hopefully land in around 10 days. :)

rcvalle added a commit to rcvalle/rust that referenced this pull request Dec 7, 2023
@rcvalle
CFI: Add char to CFI integer normalization
97032d6 
Adds char to CFI integer normalization to conform to rust-lang#118032 for
cross-language CFI support.
@rcvalle rcvalle mentioned this pull request Dec 7, 2023
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 7, 2023
@matthiaskrgr
Rollup merge of rust-lang#118719 - rcvalle:rust-cfi-normalize-integer…
71812d7 
…s-118032, r=compiler-errors

CFI: Add char to CFI integer normalization

Adds char to CFI integer normalization to conform to rust-lang#118032 for cross-language CFI support.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2023
@rust-timer
Unrolled build for rust-lang#118719
7534c1a 
Rollup merge of rust-lang#118719 - rcvalle:rust-cfi-normalize-integers-118032, r=compiler-errors

CFI: Add char to CFI integer normalization

Adds char to CFI integer normalization to conform to rust-lang#118032 for cross-language CFI support.
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 10, 2023
@rfcbot
Copy link

rfcbot commented Dec 10, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@RalfJung
Copy link
Member Author

@Mark-Simulacrum this is back in your court for review now.
@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2023
@RalfJung RalfJung removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 10, 2023
@Mark-Simulacrum
Copy link
Member

@bors r

@bors
Copy link
Contributor

bors commented Dec 11, 2023

📌 Commit b4f3f2a has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2023
@bors
Copy link
Contributor

bors commented Dec 11, 2023

⌛ Testing commit b4f3f2a with merge e299752...

@bors
Copy link
Contributor

bors commented Dec 11, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing e299752 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 11, 2023
@bors bors merged commit e299752 into rust-lang:master Dec 11, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 11, 2023
@CAD97
Copy link
Contributor

CAD97 commented Dec 11, 2023

ping @rcvalle directly as requested in #118032 (comment)

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e299752): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 0.9% [0.9%, 0.9%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.9% [0.9%, 0.9%] 1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 6.0% [6.0%, 6.0%] 1
Regressions ❌
(secondary)		 4.9% [4.9%, 4.9%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 6.0% [6.0%, 6.0%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.219s -> 668.003s (-0.03%)
Artifact size: 314.10 MiB -> 314.13 MiB (0.01%)

@RalfJung RalfJung deleted the char-u32 branch December 13, 2023 18:33
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Dec 14, 2023
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Feb 18, 2024
@he32
Add a package for rust version 1.76.0.
0e9a2a1 
Pkgsrc changes:
 * Adapt checksums and patches.

Upstream chnages:

Version 1.76.0 (2024-02-08)
==========================

Language
--------
- [Document Rust ABI compatibility between various types]
  (rust-lang/rust#115476)
- [Also: guarantee that char and u32 are ABI-compatible]
  (rust-lang/rust#118032)
- [Warn against ambiguous wide pointer comparisons]
  (rust-lang/rust#117758)

Compiler
--------
- [Lint pinned `#[must_use]` pointers (in particular, `Box<T>`
  where `T` is `#[must_use]`) in `unused_must_use`.]
  (rust-lang/rust#118054)
- [Soundness fix: fix computing the offset of an unsized field in
  a packed struct]
  (rust-lang/rust#118540)
- [Soundness fix: fix dynamic size/align computation logic for
  packed types with dyn Trait tail]
  (rust-lang/rust#118538)
- [Add `$message_type` field to distinguish json diagnostic outputs]
  (rust-lang/rust#115691)
- [Enable Rust to use the EHCont security feature of Windows]
  (rust-lang/rust#118013)
- [Add tier 3 {x86_64,i686}-win7-windows-msvc targets]
  (rust-lang/rust#118150)
- [Add tier 3 aarch64-apple-watchos target]
  (rust-lang/rust#119074)
- [Add tier 3 arm64e-apple-ios & arm64e-apple-darwin targets]
  (rust-lang/rust#115526)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------
- [Add a column number to `dbg!()`]
  (rust-lang/rust#114962)
- [Add `std::hash::{DefaultHasher, RandomState}` exports]
  (rust-lang/rust#115694)
- [Fix rounding issue with exponents in fmt]
  (rust-lang/rust#116301)
- [Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls.]
  (rust-lang/rust#117138)
- [Windows: Allow `File::create` to work on hidden files]
  (rust-lang/rust#116438)

Stabilized APIs
---------------
- [`Arc::unwrap_or_clone`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.unwrap_or_clone)
- [`Rc::unwrap_or_clone`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.unwrap_or_clone)
- [`Result::inspect`]
  (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect)
- [`Result::inspect_err`]
  (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect_err)
- [`Option::inspect`]
  (https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.inspect)
- [`type_name_of_val`]
  (https://doc.rust-lang.org/stable/std/any/fn.type_name_of_val.html)
- [`std::hash::{DefaultHasher, RandomState}`]
  (https://doc.rust-lang.org/stable/std/hash/index.html#structs)
  These were previously available only through `std::collections::hash_map`.
- [`ptr::{from_ref, from_mut}`]
  (https://doc.rust-lang.org/stable/std/ptr/fn.from_ref.html)
- [`ptr::addr_eq`](https://doc.rust-lang.org/stable/std/ptr/fn.addr_eq.html)

Cargo
-----

See [Cargo release notes]
(https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-176-2024-02-08).

Rustdoc
-------
- [Don't merge cfg and doc(cfg) attributes for re-exports]
  (rust-lang/rust#113091)
- [rustdoc: allow resizing the sidebar / hiding the top bar]
  (rust-lang/rust#115660)
- [rustdoc-search: add support for traits and associated types]
  (rust-lang/rust#116085)
- [rustdoc: Add highlighting for comments in items declaration]
  (rust-lang/rust#117869)

Compatibility Notes
-------------------
- [Add allow-by-default lint for unit bindings]
  (rust-lang/rust#112380)
  This is expected to be upgraded to a warning by default in a future Rust
  release. Some macros emit bindings with type `()` with user-provided spans,
  which means that this lint will warn for user code.
- [Remove x86_64-sun-solaris target.]
  (rust-lang/rust#118091)
- [Remove asmjs-unknown-emscripten target]
  (rust-lang/rust#117338)
- [Report errors in jobserver inherited through environment variables]
  (rust-lang/rust#113730)
  This [may warn](rust-lang/rust#120515)
  on benign problems too.
- [Update the minimum external LLVM to 16.]
  (rust-lang/rust#117947)
- [Improve `print_tts`](rust-lang/rust#114571)
  This change can break some naive manual parsing of token trees
  in proc macro code which expect a particular structure after
  `.to_string()`, rather than just arbitrary Rust code.
- [Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint]
  (rust-lang/rust#117984)
- [Vec's allocation behavior was changed when collecting some iterators]
  (rust-lang/rust#110353)
  Allocation behavior is currently not specified, nevertheless
  changes can be surprising.
  See [`impl FromIterator for Vec`]
  (https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#impl-FromIterator-for-Vec)
  for more details.
- [Properly reject `default` on free const items]
  (rust-lang/rust#117818)
@zachs18 zachs18 mentioned this pull request Apr 1, 2024
Closed
maurer pushed a commit to maurer/rust that referenced this pull request Apr 1, 2024
@rcvalle @maurer
CFI: Add char to CFI integer normalization
91699f2 
Adds char to CFI integer normalization to conform to rust-lang#118032 for
cross-language CFI support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.76.0
Development

Successfully merging this pull request may close these issues.

None yet