offset: allow zero-byte offset on arbitrary pointers

[RalfJung]

As per prior @rust-lang/opsem discussion and FCP:

• Zero-sized reads and writes are allowed on all sufficiently aligned pointers, including the null pointer
• Inbounds-offset-by-zero is allowed on all pointers, including the null pointer
• offset_from on two pointers derived from the same allocation is always allowed when they have the same address

This removes surprising UB (in particular, even C++ allows "nullptr + 0", which we currently disallow), and it brings us one step closer to an important theoretical property for our semantics ("provenance monotonicity": if operations are valid on bytes without provenance, then adding provenance can't make them invalid).

The minimum LLVM we require (v17) includes https://reviews.llvm.org/D154051, so we can finally implement this.

The offset_from change is needed to maintain the equivalence with offset: if let ptr2 = ptr1.offset(N) is well-defined, then ptr2.offset_from(ptr1) should be well-defined and return N. Now consider the case where N is 0 and ptr1 dangles: we want to still allow offset_from here.

I think we should change offset_from further, but that's a separate discussion.

Fixes #65108
Tracking issue | T-lang summary

Cc @nikic

[rustbot]

r? @cuviper

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

[RalfJung]

Uh, what...? Removing the inbounds leads to a segfault...?

[nikic]

Dropping inbounds changes optimization behavior in a really major way, which is likely to expose latent issues. Keeping inbounds for all LLVM versions is a lot less risky.

[RalfJung]

@nikic That's fine for me, I just wonder whether it can cause issues on its own when people start relying on offset(0) being allowed. LLVM was changed in two places to make that legal:

• https://reviews.llvm.org/D154054
• https://reviews.llvm.org/D154215

Could one write Rust code that is legal under the new model but miscompiled by old versions of LLVM?

How fixed is our LLVM support policy -- is it always the last 3 versions, or once LLVM 18 is released, could we say that we require LLVM 17?

[nikic]

Could one write Rust code that is legal under the new model but miscompiled by old versions of LLVM?

I think the second patch is not relevant to rust. The first one could in theory be, but you'd need some quite exotic, artificially constructed code for it. I don't think there is any chance that this would affect "real" rust code.

How fixed is our LLVM support policy -- is it always the last 3 versions, or once LLVM 18 is released, could we say that we require LLVM 17?

The earliest we can drop LLVM 16 support is when Fedora 38 goes EOL, which would be in May.

[apiraino]

If I read the prev comment correctly, this PR can switch back to the author.

@rustbot author

[RalfJung]

Nominating for t-lang discussion. This implements the t-opsem consensus from rust-lang/opsem-team#10, rust-lang/unsafe-code-guidelines#472 to generally allow zero-sized accesses on all pointers. Also see the tracking issue.

• Zero-sized reads and writes are allowed on all sufficiently aligned pointers, including the null pointer
• Inbounds-offset-by-zero is allowed on all pointers, including the null pointer
• offset_from on two pointers derived from the same allocation is always allowed when they have the same address

This means the following function is safe to be called on any pointer:

fn test_ptr(ptr: *mut ()) { unsafe {
    // Reads and writes.
    let mut val = *ptr;
    *ptr = val;
    ptr.read();
    ptr.write(());
    // Memory access intrinsics.
    // - memcpy (1st and 2nd argument)
    ptr.copy_from_nonoverlapping(&(), 1);
    ptr.copy_to_nonoverlapping(&mut val, 1);
    // - memmove (1st and 2nd argument)
    ptr.copy_from(&(), 1);
    ptr.copy_to(&mut val, 1);
    // - memset
    ptr.write_bytes(0u8, 1);
    // Offset.
    let _ = ptr.offset(0);
    let _ = ptr.offset(1); // this is still 0 bytes
    // Distance.
    let ptr = ptr.cast::<i32>();
    ptr.offset_from(ptr);
} }

Some specific concerns warrant closer scrutiny.

Null pointers

t-opsem decided to allow zero-sized reads and writes on null pointers. This is mostly for consistency: we definitely want to allow zero-sized offsets on null pointers (ptr::null::<T>().offset(0)), since this is allowed in C++ (and a proposal is being made to allow it in C) and there's no reason for us to have more UB than C++ here. But if we allow this, and therefore consider the null pointer to have a zero-sized region of "inbounds" memory, then it would be inconsistent to not allow reading from / writing to that region.

offset_from

We want to maintain the property that if let ptr2 = ptr1.offset(N) is well-defined, then ptr2.offset_from(ptr1) should be well-defined and return N. Now consider the case where N is 0 and ptr1 dangles: the pointers are derived from the same allocated object, but they are not in bounds of anything. So we need a special case saying that if they point to the same address, offset_from is allowed even if they dangle.

For now we maintain the requirement that they must be derived from the same allocation (I think we'll have to change that in the future, but let's disentangle that from the rest of this PR).

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[RalfJung]

r? @oli-obk since by far the biggest chunk of this now is interpreter changes

[bors]

⌛ Testing commit 9526ce6 with merge 11392c9...

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  SDKROOT: /Applications/Xcode_14.3.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk
  AR: ar
##[endgroup]
curl: (28) Operation too slow. Less than 100 bytes/sec transferred the last 5 seconds
Error: Failure while executing; `/usr/bin/env /usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --user-agent Homebrew/4.3.0\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 13.6.6\)\ curl/8.4.0 --header Accept-Language:\ en --fail --progress-bar --silent --remote-time --output /Users/runner/Library/Caches/Homebrew/api/formula.jws.json --location --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/4.3.0\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 13.6.6\)\ curl/8.4.0 --header Accept-Language:\ en --fail --progress-bar --silent --compressed --speed-limit 100 --speed-time 5 https://formulae.brew.sh/api/formula.jws.json` exited with 28. Here's the output:
##[error]Failure while executing; `/usr/bin/env /usr/local/Homebrew/Library/Homebrew/shims/shared/curl --disable --cookie /dev/null --globoff --user-agent Homebrew/4.3.0\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 13.6.6\)\ curl/8.4.0 --header Accept-Language:\ en --fail --progress-bar --silent --remote-time --output /Users/runner/Library/Caches/Homebrew/api/formula.jws.json --location --disable --cookie /dev/null --globoff --show-error --user-agent Homebrew/4.3.0\ \(Macintosh\;\ Intel\ Mac\ OS\ X\ 13.6.6\)\ curl/8.4.0 --header Accept-Language:\ en --fail --progress-bar --silent --compressed --speed-limit 100 --speed-time 5 https://formulae.brew.sh/api/formula.jws.json` exited with 28. Here's the output:
curl: (28) Operation too slow. Less than 100 bytes/sec transferred the last 5 seconds

##[error]Process completed with exit code 1.
Post job cleanup.

[bors]

💔 Test failed - checks-actions

[RalfJung]

@bors retry dist-apple: curl: (28) Operation too slow. Less than 100 bytes/sec transferred the last 5 seconds

[bors]

⌛ Testing commit 9526ce6 with merge 5d328a1...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk,scottmcm
Pushing 5d328a1 to master...

[rust-timer]

Finished benchmarking commit (5d328a1): 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.9%	[0.8%, 1.2%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -3.7%)

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)	-3.7%	[-3.7%, -3.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.7%	[-3.7%, -3.7%]	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: 672.314s -> 671.339s (-0.15%)
Artifact size: 315.50 MiB -> 315.69 MiB (0.06%)