Constify `is_aligned` via `align_offset` by lukas-code · Pull Request #102795 · rust-lang/rust

lukas-code · 2022-10-07T21:00:47Z

Alternative to #102753

Make align_offset work in const eval (and not always return usize::MAX) and then use that to constify is_aligned{_to}.

Tracking Issue: #104203

rustbot · 2022-10-07T21:00:51Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-10-07T21:00:53Z

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

oli-obk · 2022-10-07T22:10:32Z

I like this a lot more, thanks for doing it! I'll give it a thorough review next week

RalfJung

not commenting on whether we want this (making align_offset never-const also has some good arguments in its favor), just some feedback on the implementation

rustbot · 2022-10-08T13:01:17Z

The Miri subtree was changed

cc @rust-lang/miri

oli-obk · 2022-10-21T07:43:40Z

- pub fn is_aligned_to(self, align: usize) -> bool {
- if !align.is_power_of_two() {
- panic!("is_aligned_to: align is not a power-of-two");
+ #[rustc_const_unstable(feature = "const_pointer_is_aligned", issue = "none")]
+ pub const fn is_aligned_to(self, align: usize) -> bool {
+ assert!(align.is_power_of_two(), "is_aligned_to: align is not a power-of-two");
+
+ #[inline]
+ fn runtime(ptr: *const u8, align: usize) -> bool {
+ ptr.addr() & (align - 1) == 0
+ }
+
+ const fn comptime(ptr: *const u8, align: usize) -> bool {
+ ptr.align_offset(align) == 0
 }

- // Cast is needed for `T: !Sized`
- self.cast::<u8>().addr() & align - 1 == 0
+ // SAFETY: `ptr.align_offset(align)` returns 0 if and only if the pointer is already aligned.
+unsafe { intrinsics::const_eval_select((self.cast::<u8>(), align), comptime, runtime) }


can we always invoke ptr.align_offset(align) == 0 even at runtime? Or is that a performance concern?

There is a small performance penalty: Goldbolt link

Ah, please leave a comment to that regard

I would not call the performance penalty small. *const u8 is easy mode here, you can see the code size explode if you make the pointee type u16, and for u32 and larger align_offset is not even inlined.

For the purpose of is_aligned we can just cast the pointer to *const u8, because we only care if the offset is zero or not zero. (And we do this cast already anyway to deal with fat pointers.)

It seems inlined to me in https://rust.godbolt.org/z/b7n4MPGdf... But the difference is quite staggering:
example::is_aligned_to_old_unchecked:decrsitestrsi,rdi sete alretexample::is_aligned_to_new_unchecked:lear8,[rsi-1]test sil,3je .LBB1_1bsfrax,rsicmprax,2movecx,2 cmovb rcx,raxmovedx,-1shledx,clnotedxmovrax,-1testedx,edije .LBB1_4.LBB1_10:testrax,rax sete alret.LBB1_1:movrax,-1test dil,3jne .LBB1_10addr8,rdinegrsiandrsi,r8subrsi,rdishrrsi,2movrax,rsitestrax,rax sete alret.LBB1_4:shrrsi,clmovr10d,r8dandr10d,4shrr10,clandrdi,r8shrrdi,cllear8,[rsi-1]movr9,rsisubr9,rdimovrax,r10shrraxleardi,[rip+ .L__unnamed_1]movzxedi, byte ptr [rax+rdi]cmprsi,17jae .LBB1_6movrax,rdijmp .LBB1_9.LBB1_6:movrcx,r10imulrcx,rdimoveax,2subrax,rcximulrax,rdicmprsi,257jb .LBB1_9movedi,256.LBB1_8:imulrdi,rdimovrcx,raximulrcx,r10movedx,2subrdx,rcximulrax,rdxcmprdi,rsijb .LBB1_8.LBB1_9:andrax,r8imulrax,r9andrax,r8testrax,rax sete alret.L__unnamed_1: .ascii "\001\013\r\007\t\003\005\017"

Note that align_offset is also large enough to never get inlined (even for u8) on -Copt-level=s (and probably z too). And we definitely don't want to #[inline(always)] it due to how much code it can generate in some cases.

I found out that align_offset == 0 does get optimized to the old is_aligned_to impl with opt-level=1/2/3/s/z if you cast the pointer to *const () and #[inline] the align_offset method on pointers (not the big freestanding function): https://rust.godbolt.org/z/Kd98b9jvM
But the "optimized for size" code is still larger than the optimized for speed one, because it keeps the dead assembly for align_offset around.

RalfJung · 2022-10-21T15:38:05Z

+ self.eval_fn_call(
+ FnVal::Instance(instance),
+ (CallAbi::Rust, fn_abi),
+ &[addr, align],
+ false,
+ dest,
+ ret,
+ StackPopUnwind::NotAllowed,
+ )?;
+ Ok(ControlFlow::BREAK)


That's odd, why does this not just CONTINUE?
I guess it needs to adjust the arguments? But it is rather odd to have such different codepaths here for the two cases we can handle. I think they should be uniform.

RalfJung · 2022-10-21T15:42:15Z

- panic!("is_aligned_to: align is not a power-of-two");
+ #[rustc_const_unstable(feature = "const_pointer_is_aligned", issue = "none")]
+ pub const fn is_aligned_to(self, align: usize) -> bool {
+ assert!(align.is_power_of_two(), "is_aligned_to: align is not a power-of-two");


This will be a slightly uglier panic message than before, since it will also print the stringified expression.

There doesn't seem to be a (significant) difference to me, but I've changed it back for now. (Goldbolt diff)

lukas-code · 2022-10-22T21:33:58Z

I've updated it now to never actually call align_offset (the lang item) during const eval and instead use a slightly simplified and de-obfuscated function align_offset_impl that runs entirely in the interpreter hook.

Also I added docs and a bunch of examples to is_aligned{,_to} to explain how these functions work at runtime and at comptime. I've also put a disclaimer that comptime alignment is super-unstable and subject to change. Docs demo available here. (The doctests are only ignored for stage0)

This reverts commit f3a577bfae376c0222e934911865ed14cddd1539.

Co-authored-by: Ralf Jung <post@ralfj.de>

* fix allocation alignment for 16bit platforms * add edge case where `stride % align != 0` on pointers with provenance

lukas-code · 2022-11-19T16:02:29Z

Rebased and dropped 7e1481997b8bdf94e11a59236a17100eeca5633e since #103378 got merged.

oli-obk · 2022-11-19T16:20:03Z

@bors r+

bors · 2022-11-19T16:20:06Z

📌 Commit c9c017d has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-11-19T18:57:42Z

⌛ Testing commit c9c017d with merge c5d82ed...

bors · 2022-11-19T21:54:48Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c5d82ed to master...

rust-timer · 2022-11-19T23:40:30Z

Finished benchmarking commit (c5d82ed): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.0%	[0.8%, 1.1%]	2
Regressions ❌ (secondary)	3.4%	[3.0%, 3.8%]	2
Improvements ✅ (primary)	-3.6%	[-3.6%, -3.6%]	1
Improvements ✅ (secondary)	-4.2%	[-4.2%, -4.2%]	1
All ❌✅ (primary)	-0.6%	[-3.6%, 1.1%]	3

Cycles

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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-4.0%, -3.7%]	2
All ❌✅ (primary)	-	-	0

RalfJung · 2022-11-20T09:51:33Z

+ // The cast to `()` is used to
+ // 1. deal with fat pointers; and
+ // 2. ensure that `align_offset` doesn't actually try to compute an offset.
+ self.cast::<()>().align_offset(align) == 0


Sadly this caused a regression in Miri: rust-lang/miri#2682

While the immediate issue is fixed, it's still somewhat strange that is_aligned would change behavior with Miri's symbolic alignment mode... but maybe it makes sense, it is consistent with align_to, anyway. We'll have to watch out for other similar regressions. If too many bugreports come in we'll have to find another solution.

The last line of this doc test is also failing with -Zmiri-symbolic-alignment-check
rust/library/core/src/ptr/const_ptr.rs
Lines 1492 to 1511 in c9c017d
/// ```
/// #![feature(pointer_is_aligned)]
/// #![feature(pointer_byte_offsets)]
///
/// // On some platforms, the alignment of i32 is less than 4.
/// #[repr(align(4))]
/// struct AlignedI32(i32);
///
/// let data = AlignedI32(42);
/// let ptr = &data as *const AlignedI32;
///
/// assert!(ptr.is_aligned_to(1));
/// assert!(ptr.is_aligned_to(2));
/// assert!(ptr.is_aligned_to(4));
///
/// assert!(ptr.wrapping_byte_add(2).is_aligned_to(2));
/// assert!(!ptr.wrapping_byte_add(2).is_aligned_to(4));
///
/// assert_ne!(ptr.is_aligned_to(8), ptr.wrapping_add(1).is_aligned_to(8));
/// ```
Maybe we should partially revert daccb8c to put the const_eval_select back?
It might also make sense to redefine -Zmiri-symbolic-alignment-check as "runtime alignment behaves like const eval alignment", because i think that is what it currently does after this PR and rust-lang/miri#2683.

We are not running libcore tests with symbolic alignment so I guess I didn't notice this.
It might also make sense to redefine -Zmiri-symbolic-alignment-check as "runtime alignment behaves like const eval alignment", because i think that is what it currently does after this PR and rust-lang/miri#2683.
I guess that makes sense. Are you proposing just a docs change or also an implementation change?

We could do some deduplication between the ctfe impl and miri impl of align_offset, but the implementation looks functionally identical to me, so this would mostly be a docs change.
Currently, the docs look like this:
-Zmiri-symbolic-alignment-check makes the alignment check more strict. By default, alignment is checked by casting the pointer to an integer, and making sure that is a multiple of the alignment. This can lead to cases where a program passes the alignment check by pure chance, because things "happened to be" sufficiently aligned -- there is no UB in this execution but there would be UB in others. To avoid such cases, the symbolic alignment check only takes into account the requested alignment of the relevant allocation, and the offset into that allocation. This avoids missing such bugs, but it also incurs some false positives when the code does manual integer arithmetic to ensure alignment. (The standard library align_to method works fine in both modes; under symbolic alignment it only fills the middle slice when the allocation guarantees sufficient alignment.)
From this it actually seems pretty clear to me that new behavior for is_aligned with -Zmiri-symbolic-alignment-check is correct and the old one was wrong. ~~Also, it seems weird to me that the docs don't mention align_offset at all when literally all this flag does is change the behavior of align_offset.~~
Maybe we could just change the last sentence in parentheses to something like
(This changes the runtime behavior of alignment-related standard library functions like is_aligned, align_offset, or align_to to match the compiletime behavior. For example, align_to only fills the middle slice when the allocation guarantees sufficient alignment.)

Also, it seems weird to me that the docs don't mention align_offset at all when literally all this flag does is change the behavior of align_offset.
It does more. It's core feature is to toggle a flag in the interpreter that affects how alignment checking works. Adjusting align_offset is just a side thing that we also do to keep more code working in this mode.

…lign-offset, r=oli-obk Constify `is_aligned` via `align_offset` Alternative to rust-lang#102753 Make `align_offset` work in const eval (and not always return `usize::MAX`) and then use that to constify `is_aligned{_to}`. Tracking Issue: rust-lang#104203

rustbot added 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 Oct 7, 2022

rust-highfive assigned Mark-Simulacrum Oct 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 7, 2022

lukas-code mentioned this pull request Oct 7, 2022
Constify pointer::is_aligned{,to}#102753
Closed

This comment has been minimized.
Sign in to view

lukas-code force-pushed the constify-is-aligned-via-align-offset branch from d6732ea to 51474dcCompare October 7, 2022 21:45

This comment has been minimized.
Sign in to view

RalfJung reviewed Oct 8, 2022
View reviewed changes

lukas-code force-pushed the constify-is-aligned-via-align-offset branch 2 times, most recently from 5b35859 to e261e55Compare October 8, 2022 13:01

Mark-Simulacrum assigned oli-obk and unassigned Mark-Simulacrum Oct 16, 2022

oli-obk reviewed Oct 20, 2022
View reviewed changes

Comment threadlibrary/core/src/ptr/mod.rs Outdated

RalfJung mentioned this pull request Oct 20, 2022
Alignment at const time? rust-lang/unsafe-code-guidelines#370
Open

lukas-code force-pushed the constify-is-aligned-via-align-offset branch 2 times, most recently from 6f1df27 to c2605f6Compare October 20, 2022 19:40

oli-obk requested changes Oct 21, 2022
View reviewed changes

RalfJung reviewed Oct 21, 2022
View reviewed changes

Comment threadcompiler/rustc_const_eval/src/const_eval/machine.rs Outdated

RalfJung reviewed Oct 21, 2022
View reviewed changes

Comment threadcompiler/rustc_const_eval/src/const_eval/machine.rs Outdated

RalfJung reviewed Oct 21, 2022
View reviewed changes

lukas-code mentioned this pull request Oct 21, 2022
align_offset infinite loop #103361
Closed

lukas-code force-pushed the constify-is-aligned-via-align-offset branch from be86396 to 166fb94Compare October 22, 2022 19:43

lukas-code force-pushed the constify-is-aligned-via-align-offset branch from 8d90187 to 005f92dCompare October 23, 2022 11:09

Lukas Markeffskyand others added 4 commits November 19, 2022 16:58

Revert "don't call align_offset during const eval, ever"
3d7e9c4
This reverts commit f3a577bfae376c0222e934911865ed14cddd1539.

Update comment on pointer-to-usize transmute
e90d15b
Co-authored-by: Ralf Jung <post@ralfj.de>

fix assembly test on apple
53c2ee8

update provenance test
c9c017d
* fix allocation alignment for 16bit platforms * add edge case where `stride % align != 0` on pointers with provenance

lukas-code force-pushed the constify-is-aligned-via-align-offset branch from f862443 to c9c017dCompare November 19, 2022 16:02

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 19, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2022

bors merged commit c5d82ed into rust-lang:masterNov 19, 2022

rustbot added this to the 1.67.0 milestone Nov 19, 2022

rustbot removed the perf-regression Performance regression. label Nov 19, 2022

lukas-code deleted the constify-is-aligned-via-align-offset branch November 20, 2022 09:30

RalfJung mentioned this pull request Nov 20, 2022
is_aligned goes wrong on non-provenance pointers with symbolic alignment rust-lang/miri#2682
Closed

RalfJung reviewed Nov 20, 2022
View reviewed changes

Robert-Cunningham mentioned this pull request Jan 17, 2023
Make traits_in_crate and impls_in_crate proper queries #100601
Closed
4 tasks

matthiaskrgr mentioned this pull request Aug 3, 2023
[ICE]: Broken MIR in drop glue with specialization #107228
Closed
4 tasks

RalfJung mentioned this pull request Nov 12, 2023
Tracking Issue for const_align_offset#90962
Closed
5 tasks

	/// ```
	/// #![feature(pointer_is_aligned)]
	/// #![feature(pointer_byte_offsets)]
	///
	/// // On some platforms, the alignment of i32 is less than 4.
	/// #[repr(align(4))]
	/// struct AlignedI32(i32);
	///
	/// let data = AlignedI32(42);
	/// let ptr = &data as *const AlignedI32;
	///
	/// assert!(ptr.is_aligned_to(1));
	/// assert!(ptr.is_aligned_to(2));
	/// assert!(ptr.is_aligned_to(4));
	///
	/// assert!(ptr.wrapping_byte_add(2).is_aligned_to(2));
	/// assert!(!ptr.wrapping_byte_add(2).is_aligned_to(4));
	///
	/// assert_ne!(ptr.is_aligned_to(8), ptr.wrapping_add(1).is_aligned_to(8));
	/// ```

Uh oh!

Uh oh!

Conversation

lukas-code commented Oct 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 7, 2022

Uh oh!

rust-highfive commented Oct 7, 2022

Uh oh!

This comment has been minimized.

oli-obk commented Oct 7, 2022

Uh oh!

This comment has been minimized.

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustbot commented Oct 8, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJungOct 21, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukas-code commented Oct 22, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukas-code commented Nov 19, 2022

Uh oh!

oli-obk commented Nov 19, 2022

Uh oh!

bors commented Nov 19, 2022

Uh oh!

bors commented Nov 19, 2022

Uh oh!

bors commented Nov 19, 2022

Uh oh!

rust-timer commented Nov 19, 2022

Overall result: no relevant changes - no action needed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukas-codeNov 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

lukas-code commented Oct 7, 2022•
edited
Loading

RalfJungOct 21, 2022•
edited
Loading

lukas-code commented Oct 22, 2022•
edited
Loading

lukas-codeNov 20, 2022•
edited
Loading

RalfJungNov 20, 2022•
edited
Loading