Emit llvm dereferenceable in more cases on newer LLVM versions

[WaffleLapkin]

View all comments

On llvm >= 23 emit dereferenceable even without nofree, since llvm/llvm-project#204795 makes dereferenceable act "at a point" / not imply nofree.

Additionally restrict dereferenceable to &Freeze / &mut Unpin / Box<Unpin, _> only (this is only relevant for llvm >= 23 as otherwise !Freeze/!Unpin suppress nofree and as such dereferenceable).

r? nikic

I thought this will be a bit harder change, but since your refactoring in #156281 it's actually trivial ^^'

Draft because I don't think this can be tested prior to LLVM update. I'm also not sure if the version check I did is the appropriate one...

cc @RalfJung

[nikic]

Yeah, we should wait until the LLVM 23 upgrade so we can properly test this.

View changes since this review

[RalfJung]

With LLVM 23 we can also put dereferenceable on &Cell arguments, on &mut !Unpin arguments, and on &T/&mut T return types for all T. Does this PR do all that?

[WaffleLapkin]

@RalfJung yes. What currently stops dereferenceable from being applied to &!Freeze and &mut !Unpin arguments, and references in return types, is that in all these cases we don't set NoFree:

rust/compiler/rustc_ty_utils/src/abi.rs

Lines 406 to 424 in f10db29

	// NoFree is not valid on return values. If it were, it would mean something like
	// "will not be freed until the end of the program", which is generally not valid for
	// references.
	let no_free = !is_return
	&& match kind {
	// Non-frozen shared references are not necessarily dereferenceable for the
	// entire duration of the function
	// (see <https://github.com/rust-lang/rust/pull/98017>).
	PointerKind::SharedRef { frozen } => frozen,
	// Mutable references to potentially self-referential types are not necessarily
	// dereferenceable for the entire duration of the function
	// (see <https://github.com/rust-lang/unsafe-code-guidelines/issues/381>).
	PointerKind::MutableRef { unpin } => unpin,
	// Box may be deallocated during execution of the function.
	PointerKind::Box { .. } => false,
	};
	if no_free {
	attrs.set(ArgAttribute::NoFree);
	}

With NoFree no longer being required, they will all become dereferenceable.

I suppose another way to put this is that rustc already uses dereferenceable-at-a-point semantics since #156281, and applies dereferenceable-at-a-point everywhere it's valid. The only check that prevents llvm dereferenceable from being applied in cases you mentioned, is the one that I'm changing in this PR.

[RalfJung]

Nice, that's what I was hoping for. :)

This will be much easier to review once we actually have LLVM 23 and can look at the codegen test diff.

[RalfJung]

Looks great :)

View changes since this review

[WaffleLapkin]

@RalfJung I'm confused by this. You said applying dereferenceable on &!Freeze / &mut !Unpin should be fine now, but rust-lang/unsafe-code-guidelines#381 seems to disagree with that. Was there some kind of update since that issue was written or am I missing something else?

View changes since the review

[RalfJung]

Ah... good point. I had forgotten about this. This also relates to an old LLVM semantics question that never got resolved.

In the end the question is what dereferenceable really means to LLVM. Does it mean "this memory is in-bounds of some allocation" or does it mean "pretend we did a byte-type read here". The former is fine, Miri checks that. The latter is not fine as that read can affect the aliasing model (and the data race model, but LLVM ~defines that problem away and this is not our worst crime on the memory model side so we hope it's fine).

So what is unambiguously correct is to add dereferenceable to &i32 and &mut i32 return types, and to Box<i32> arguments and return types. But for !Freeze or !Unpin things are indeed less clear.

[WaffleLapkin]

Changed to PR such that it only emits dereferenceable in cases when miri performs implicit reads. #158863 (comment).

[WaffleLapkin]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 69d43e7 (69d43e79223ff1e0056f7f333df9005f685ab5ae)
Base parent: b070f45 (b070f45ad92ed45b20e57d9483a21657d0d00715)

[rust-timer]

Finished benchmarking commit (69d43e7): comparison URL.

Overall result: ❌ regressions - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.2%, 2.5%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 4.3%, secondary -1.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Cycles

Results (primary -2.4%, secondary 4.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.1%	[2.8%, 5.3%]	2
Improvements ✅ (primary)	-2.4%	[-2.4%, -2.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-2.4%, -2.4%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 459.982s -> 458.725s (-0.27%)
Artifact size: 398.64 MiB -> 399.05 MiB (0.10%)

[WaffleLapkin]

Regression feels like noise to me. match-stress regresses in check profile too, which can't be caused by llvm codegen changes here... Unless the match checking in rustc is being optimized worse somehow, in which case I'm not sure what can be done about it.

[rustbot]

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

[WaffleLapkin]

After a discussion with @RalfJung, it seems clear that &!Freeze / &mut !Unpin references shouldn't be dereferenceable. I added a commit to account for this.

After that the semantics changes in this PR (given llvm >= 23) are:

• Box<Unpin, _> arguments and return types gain dereferenceable
• &Freeze and &mut Unpin return types gain dereferenceable

@rustbot ready

[RalfJung]

Hm, the entire point of having frozen in the PointerKind was that dealing with this is handled later, when this is translated into LLVM attributes. Seems a bit messy to have some of the frozen logic here and some of it very far away in a different part of the compiler -- can't we avoid that?

View changes since the review

[RalfJung]

I think that place might be wherever PointeeInfo gets turned into ArgAttributes.

[WaffleLapkin]

So, I'm kind of on the fence about this...

In #150447 (and specifically 312055f) I made it so size is always correct, independently of safe pointer kind. I like this because it means that there isn't a weird invariant where interpretation of a value of the field (of PointeeInfo) depends on the value of another field.

I could move this into arg_attrs_for_rust_scalar, but that would mean that meaning of size is entangled with safe again...

I do see your point though, not sure what is the best separation to make here.

[RalfJung]

In #150447 (and specifically 312055f) I made it so size is always correct, independently of safe pointer kind. I like this because it means that there isn't a weird invariant where interpretation of a value of the field (of PointeeInfo) depends on the value of another field.

Yeah, that's exactly why this PR here feels like a step back now.

[RalfJung]

The underlying problem is that there are (at least) two notions of dereferenceable(N):

1. There exist N bytes of in-bounds memory starting at that pointer.
2. We can insert a load of N bytes without introducing UB.

(1) is true for all references, but we only want to emit LLVM dereferenceable for (2). We need to decide what PointeeInfo::size means. Given that (1) is not really relevant for codegen at all, it's just a thing Miri does internally, IMO it doesn't make much sense to represent (1) explicitly in the compiler at this time, so we should change the meaning of PointeeInfo::size to (2).

... well I think I just argued for what the PR does, and against what I wrote above. I hate it when that happens. 😂

[RalfJung]

FWIW the size field docs for PointeeInfo will also need changing.

On a function argument, “dereferenceable” here means “dereferenceable for the entire duration of this function call”, i.e. it is UB for the memory that this pointer points to be freed while this function is still running.

View changes since the review

[WaffleLapkin]

Does this sound good to you?

rust/compiler/rustc_abi/src/lib.rs

Lines 2354 to 2368 in ccaaed3

	/// If `size` is not zero, then the pointer is either null or dereferenceable for this many bytes
	/// (independent of `safe`).
	///
	/// On a function argument, "dereferenceable" here means "dereferenceable at the function entry",
	/// i.e. it is sound for the memory that this pointer points to be freed (unless `safe` restrics
	/// freeing, such is the case for references).
	///
	/// On a function return, "dereferenceable" here means "dereferenceable at the moment of return
	/// from the function".
	///
	/// Note that in either case dereferenceability implies that it's sound to add a speculative
	/// (spurious) read of the pointer. The backend is free to add such reads at will. (this is
	/// useful to hoist pointer reads out of loops without proving that the loop executes at least
	/// once, for example)
	pub size: Size,

[RalfJung]

I'd put the new speculative read part at the beginning.

And then I think we can just have a brief comment saying that this can be used in argument and return position and in both cases it means "dereferenceable right now (but could be freed any time later)".

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[rust-log-analyzer]

The job test-tidy failed! Check out the build log: (web) (plain enhanced) (plain)

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

finished building tool typos
error: `restrics` should be `restricts`
     ╭▸ compiler/rustc_abi/src/lib.rs:2358:93
     │
2358 │     /// i.e. it is sound for the memory that this pointer points to be freed (unless `safe` restrics
     ╰╴                                                                                            ━━━━━━━━
rerun with `--bless` to fix typos: `./x.py test tidy --extra-checks=spellcheck --bless`
tidy [extra_checks:spellcheck]: checks with external tool 'typos' failed
tidy [extra_checks:spellcheck]: FAIL
yarn install v1.22.22
warning package.json: No license field
warning ../../package.json: License should be a valid SPDX license expression
warning No license field
[1/4] Resolving packages...
---
Running eslint on rustdoc JS files
info: ES-Check: checking 7 files...
info: ✓ ES-Check passed! All files are ES10 compatible.
typechecking javascript files
tidy: The following check failed: extra_checks:spellcheck
Command `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools-bin/rust-tidy --root-path=/checkout --cargo-path=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo --output-dir=/checkout/obj/build --concurrency=4 --npm-path=/node/bin/yarn --ci=true --extra-checks=py,cpp,js,spellcheck` failed with exit code 1
Created at: src/bootstrap/src/core/build_steps/tool.rs:1627:23
Executed at: src/bootstrap/src/core/build_steps/test.rs:1747:29

Command has failed. Rerun with -v to see more details.
Bootstrap failed while executing `test src/tools/tidy tidyselftest --extra-checks=py,cpp,js,spellcheck`
Currently active steps:
test::Tidy {  } at src/bootstrap/src/core/build_steps/test.rs:1665
Build completed unsuccessfully in 0:02:03
  local time: Wed Sep 30 09:40:38 UTC 2026

[RalfJung]

This should refer to the semantics as documented on PointeeInfo. What LLVM does is only indirectly relevant here.

View changes since the review