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.

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

const-eval: make misalignment a hard error #115524

Merged
merged 2 commits into from
Oct 14, 2023
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Sep 4, 2023

It's been a future-incompat error (showing up in cargo's reports) since #104616, Rust 1.68, released in March. That should be long enough.

The question for the lang team is simply -- should we move ahead with this, making const-eval alignment failures a hard error? (It turns out some of them accidentally already were hard errors since #104616. But not all so this is still a breaking change. Crater found no regression.)

@rustbot
Copy link
Collaborator

rustbot commented Sep 4, 2023

r? @wesleywiser

(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. labels Sep 4, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 4, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

The Miri subtree was changed

cc @rust-lang/miri

let ptr = mem.as_ptr().byte_add(1);
let _val = *ptr; //~ERROR: evaluation of constant value failed
//~^NOTE: accessing memory with alignment 1, but alignment 4 is required
};
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@RalfJung RalfJung Sep 4, 2023

Choose a reason for hiding this comment

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

Ah, here's why:

let align =
if M::enforce_alignment(self).should_check() { mplace.align } else { Align::ONE };
self.check_ptr_access_align(mplace.ptr, size, align, CheckInAllocMsg::DerefTest)?;

check_ptr_access_align will always check alignment, so this entirely circumvented the future-incompat machinery.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 4, 2023

Should we crater it again just for impact analysis?

@oli-obk oli-obk added T-lang Relevant to the language team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 4, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023 via email

@oli-obk oli-obk added the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 4, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023

This is a kind of funny FCP since it turns out the breaking change accidentally happened in #104616, we're now just removing some dead code.^^

@oli-obk
Copy link
Contributor

oli-obk commented Sep 4, 2023

r=me on the impl.

Nominating for T-lang team discussion. Don't know if you want to FCP this, as it's already in line with the const UB RFC (if your code exhibits UB at compile-time, but passes, we are allowed to break it later)

@RalfJung RalfJung removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Sep 4, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023

I'd say let's nominate, and then T-lang can decide if they want an FCP or not.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023

Ah, the lint docs actually have an example of code that did trigger the lint. So this is a breaking change after all.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023

@bors try

@bors
Copy link
Contributor

bors commented Sep 4, 2023

⌛ Trying commit 70db9968a8d128fde5d2fb656c7b4a62cf3c92be with merge bbf68c7b2a40f9417a5515b471f1ff6e35782bca...

@bors
Copy link
Contributor

bors commented Sep 4, 2023

💔 Test failed - checks-actions

@bors bors 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 Sep 4, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 4, 2023

All jobs are failing with Error: Can't use 'tar -xzf' extract archive file: /home/runner/work/_actions/_temp_4fa9e092-7404-456e-a7a7-7f3b4643ea20/643dcc28-3f26-4cc7-ad85-3ef6e4c709eb.tar.gz. return code: 2, not just this PR.

@rust-log-analyzer

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2023
don't UB on dangling ptr deref, instead check inbounds on projections

This implements rust-lang/reference#1387 in Miri. See that PR for what the change is about.

Detecting dangling references in `let x = &...;` is now done by validity checking only, so some tests need to have validity checking enabled. There is no longer inherently a "nodangle" check in evaluating the expression `&*ptr` (aside from the aliasing model).

r? `@oli-obk`

Blocked on:
- rust-lang/reference#1387
- rust-lang#115524
@RalfJung
Copy link
Member Author

@joshtriplett @pnkfelix @scottmcm friendly reminder that there's a checkbox waiting for you here. :)

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 3, 2023
@rfcbot
Copy link

rfcbot commented Oct 3, 2023

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

@tmandry tmandry removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 10, 2023
@traviscross
Copy link
Contributor

@rustbot labels -S-waiting-on-team

@rustbot rustbot removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Oct 10, 2023
@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 Oct 13, 2023
@rfcbot
Copy link

rfcbot commented Oct 13, 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

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Oct 13, 2023

📌 Commit bd33846 has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 13, 2023
@bors
Copy link
Contributor

bors commented Oct 14, 2023

⌛ Testing commit bd33846 with merge 75a5dd0...

@bors
Copy link
Contributor

bors commented Oct 14, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 75a5dd0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 14, 2023
@bors bors merged commit 75a5dd0 into rust-lang:master Oct 14, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 14, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (75a5dd0): 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
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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)
1.4% [1.1%, 1.5%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [1.1%, 1.5%] 3

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: 627.259s -> 628.136s (0.14%)
Artifact size: 271.34 MiB -> 271.27 MiB (-0.02%)

@RalfJung RalfJung deleted the misalign branch October 14, 2023 08:16
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2023
don't UB on dangling ptr deref, instead check inbounds on projections

This implements rust-lang/reference#1387 in Miri. See that PR for what the change is about.

Detecting dangling references in `let x = &...;` is now done by validity checking only, so some tests need to have validity checking enabled. There is no longer inherently a "nodangle" check in evaluating the expression `&*ptr` (aside from the aliasing model).

r? `@oli-obk`

Based on:
- rust-lang/reference#1387
- rust-lang#115524
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 17, 2023
don't UB on dangling ptr deref, instead check inbounds on projections

This implements rust-lang/reference#1387 in Miri. See that PR for what the change is about.

Detecting dangling references in `let x = &...;` is now done by validity checking only, so some tests need to have validity checking enabled. There is no longer inherently a "nodangle" check in evaluating the expression `&*ptr` (aside from the aliasing model).

r? `@oli-obk`

Based on:
- rust-lang/reference#1387
- rust-lang/rust#115524
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <[email protected]>
Co-authored-by: Colin Finck <[email protected]>
Co-authored-by: Ian Jackson <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Trevor Gross <[email protected]>
Co-authored-by: Evan Merlock <[email protected]>
Co-authored-by: joboet <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: Mark Rousskov <[email protected]>
Co-authored-by: onur-ozkan <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: The 8472 <[email protected]>
Co-authored-by: Samuel Thibault <[email protected]>
Co-authored-by: reez12g <[email protected]>
Co-authored-by: Jakub Beránek <[email protected]>
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Oct 19, 2023
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 3, 2024
Pkgsrc changes:
 * Adjust patches and cargo checksums to new versions.
 * For an external LLVM, set dependency of llvm >= 16, in accordance
   with the upstream changes.
 * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported.
 * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded
   LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it,
   resulting in an illegal instruction fault during the build.
   Ref. rust-lang/rust#117231

Upstream changes:

Version 1.75.0 (2023-12-28)
==========================

- [Stabilize `async fn` and return-position `impl Trait` in traits.]
  (rust-lang/rust#115822)
- [Allow function pointer signatures containing `&mut T` in `const` contexts.]
  (rust-lang/rust#116015)
- [Match `usize`/`isize` exhaustively with half-open ranges.]
  (rust-lang/rust#116692)
- [Guarantee that `char` has the same size and alignment as `u32`.]
  (rust-lang/rust#116894)
- [Document that the null pointer has the 0 address.]
  (rust-lang/rust#116988)
- [Allow partially moved values in `match`.]
  (rust-lang/rust#103208)
- [Add notes about non-compliant FP behavior on 32bit x86 targets.]
  (rust-lang/rust#113053)
- [Stabilize ratified RISC-V target features.]
  (rust-lang/rust#116485)

Compiler
--------

- [Rework negative coherence to properly consider impls that only
  partly overlap.] (rust-lang/rust#112875)
- [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.]
  (rust-lang/rust#116493)
- [Consider alias bounds when computing liveness in NLL.]
  (rust-lang/rust#116733)
- [Add the V (vector) extension to the `riscv64-linux-android` target spec.]
  (rust-lang/rust#116618)
- [Automatically enable cross-crate inlining for small functions]
  (rust-lang/rust#116505)
- Add several new tier 3 targets:
    - [`csky-unknown-linux-gnuabiv2hf`]
      (rust-lang/rust#117049)
    - [`i586-unknown-netbsd`]
      (rust-lang/rust#117170)
    - [`mipsel-unknown-netbsd`]
      (rust-lang/rust#117356)

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

Libraries
---------

- [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.]
  (rust-lang/rust#96979)
- [Implement `BufRead` for `VecDeque<u8>`.]
  (rust-lang/rust#110604)
- [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.]
  (rust-lang/rust#110729)
- [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.]
  (rust-lang/rust#113747)
- [Implement `Default` for `ExitCode`.]
  (rust-lang/rust#114589)
- [Guarantee representation of None in NPO]
  (rust-lang/rust#115333)
- [Document when atomic loads are guaranteed read-only.]
  (rust-lang/rust#115577)
- [Broaden the consequences of recursive TLS initialization.]
  (rust-lang/rust#116172)
- [Windows: Support sub-millisecond sleep.]
  (rust-lang/rust#116461)
- [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl]
  (rust-lang/rust#100806)
- [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.]
  (rust-lang/rust#115108)

Stabilized APIs
---------------

- [`Atomic*::from_ptr`]
  (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr)
- [`FileTimes`]
  (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html)
- [`FileTimesExt`]
  (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html)
- [`File::set_modified`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified)
- [`File::set_times`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times)
- [`IpAddr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical)
- [`Ipv6Addr::to_canonical`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical)
- [`Option::as_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice)
- [`Option::as_mut_slice`]
  (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice)
- [`pointer::byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add)
- [`pointer::byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset)
- [`pointer::byte_offset_from`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from)
- [`pointer::byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub)
- [`pointer::wrapping_byte_add`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add)
- [`pointer::wrapping_byte_offset`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset)
- [`pointer::wrapping_byte_sub`]
  (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub)

These APIs are now stable in const contexts:

- [`Ipv6Addr::to_ipv4_mapped`]
  (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped)
- [`MaybeUninit::assume_init_read`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read)
- [`MaybeUninit::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed)
- [`mem::discriminant`]
  (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html)
- [`mem::zeroed`]
  (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html)

Cargo
-----

- [Add new packages to `[workspace.members]` automatically.]
  (rust-lang/cargo#12779)
- [Allow version-less `Cargo.toml` manifests.]
  (rust-lang/cargo#12786)
- [Make browser links out of HTML file paths.]
  (rust-lang/cargo#12889)

Rustdoc
-------

- [Accept less invalid Rust in rustdoc.]
  (rust-lang/rust#117450)
- [Document lack of object safety on affected traits.]
  (rust-lang/rust#113241)
- [Hide `#[repr(transparent)]` if it isn't part of the public ABI.]
  (rust-lang/rust#115439)
- [Show enum discriminant if it is a C-like variant.]
  (rust-lang/rust#116142)

Compatibility Notes
-------------------

- [FreeBSD targets now require at least version 12.]
  (rust-lang/rust#114521)
- [Formally demote tier 2 MIPS targets to tier 3.]
  (rust-lang/rust#115238)
- [Make misalignment a hard error in `const` contexts.]
  (rust-lang/rust#115524)
- [Fix detecting references to packed unsized fields.]
  (rust-lang/rust#115583)
- [Remove support for compiler plugins.]
  (rust-lang/rust#116412)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.