Tagged pointers, now with strict provenance! #110243

WaffleLapkin · 2023-04-12T16:27:53Z

This is a big refactor of tagged pointers in rustc, with three main goals:

Porting the code to the strict provenance
Cleanup the code
Document the code (and safety invariants) better

This PR has grown quite a bit (almost a complete rewrite at this point...), so I'm not sure what's the best way to review this, but reviewing commit-by-commit should be fine.

r? @Nilstrieb

fat pointers rule!

…rs directly

Does not really matter, but may be nicer in case the pointer has some specific debug impl.

Just use `deref{,_mut}`!

why can't I _ it :'(

Noratrieb

I'd like to see a few more tests for Eq, Hash and HasStable (asserting that a packed ptr and tag hash the same as (ptr, tag).

I have a few other comments, but this looks great!

compiler/rustc_data_structures/src/tagged_ptr.rs

Noratrieb · 2023-04-13T16:58:05Z

compiler/rustc_data_structures/src/tagged_ptr.rs

 ///
-/// No more than `BITS` least significant bits may be set in the returned usize.
+/// [`BITS`]: Tag::BITS
 pub unsafe trait Tag: Copy {


I wonder whether it would be worth it to make this trait safe with an assert!(value.into_usize() < (1 << BITS)). That should be optimized away from all the uses I can imagine, although it would have to be tested.

We could even assert this at compile time using extremely ugly beautiful hacks like https://godbolt.org/z/fobx1szMq, although I am not sure whether this would be a good idea (almost certainly not).

compiler/rustc_data_structures/src/tagged_ptr.rs

compiler/rustc_data_structures/src/tagged_ptr/copy.rs

compiler/rustc_data_structures/src/tagged_ptr/copy/tests.rs

compiler/rustc_data_structures/src/tagged_ptr/drop/tests.rs

compiler/rustc_data_structures/src/aligned.rs

compiler/rustc_data_structures/src/tagged_ptr.rs

scottmcm · 2023-04-14T01:12:44Z

compiler/rustc_data_structures/src/tagged_ptr/copy.rs

+    }
+
+    const TAG_BIT_SHIFT: usize = usize::BITS as usize - T::BITS;
+    const ASSERTION: () = { assert!(T::BITS <= P::BITS) };


This is a nice example of a reasonable non-local error 👍

compiler/rustc_data_structures/src/tagged_ptr/copy/tests.rs

compiler/rustc_data_structures/src/tagged_ptr/copy.rs

bors · 2023-04-14T14:35:18Z

⌛ Trying commit 5571dd0 with merge 9d28dbbaf465174d2a9b082af86bace14c32a9b9...

bors · 2023-04-14T16:19:24Z

☀️ Try build successful - checks-actions
Build commit: 9d28dbbaf465174d2a9b082af86bace14c32a9b9 (9d28dbbaf465174d2a9b082af86bace14c32a9b9)

rust-timer · 2023-04-15T07:21:36Z

Finished benchmarking commit (9d28dbbaf465174d2a9b082af86bace14c32a9b9): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -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.8%	[0.7%, 1.3%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

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

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)	2.9%	[2.9%, 2.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.3%, -2.2%]	2
All ❌✅ (primary)	-	-	0

Noratrieb · 2023-04-17T12:13:23Z

@bors r+

bors · 2023-04-17T12:13:25Z

📌 Commit 5571dd0 has been approved by Nilstrieb

It is now in the queue for this repository.

bors · 2023-04-17T19:23:46Z

⌛ Testing commit 5571dd0 with merge c95c09c48438d37071735988dcb412d29557a811...

bors · 2023-04-17T19:31:38Z

💔 Test failed - checks-actions

Noratrieb · 2023-04-17T19:39:05Z

the server was not having a happy day
@bors retry

rust-log-analyzer · 2023-04-17T19:39:14Z

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

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

bors · 2023-04-17T21:50:16Z

⌛ Testing commit 5571dd0 with merge 7908a1d...

bors · 2023-04-17T23:58:25Z

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 7908a1d to master...

rust-timer · 2023-04-18T02:10:52Z

Finished benchmarking commit (7908a1d): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.6%	[0.5%, 0.7%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.5%, -0.5%]	1
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)	3.9%	[3.9%, 3.9%]	1
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	3.9%	[3.9%, 3.9%]	1

Cycles

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

WaffleLapkin added 15 commits April 11, 2023 19:18

Add bits_for helper for tagged pointers & fixup docs

c738dcc

Sprinkle some whitespace & uses

f028636

Bless tagged pointers (comply to strict provenance)

3c6f4c1

Refactor tagged ptr packing into a function

12fd610

Fix doc test

ad92677

Remove useless parameter from ghost

26232f1

Lift Pointer's requirement for the pointer to be thin

9051331

fat pointers rule!

Remove Pointer::with_ref in favour implementing it on tagged pointe…

c6acd5c

…rs directly

Make tagged pointers debug impls print the pointer

c7c0b85

Does not really matter, but may be nicer in case the pointer has some specific debug impl.

Remove pointer_{ref,mut} from tagged pointers

8f40820

Just use `deref{,_mut}`!

Shorten COMPARE_PACKED => CP where it is not important

3df9a7b

why can't I _ it :'(

Move code around

6f64ae3

Add TaggedPtr::set_tag

5e4577e

Add tests for tagged pointers

6f9b15c

Document tagged pointers better

838c549

rustbot assigned Noratrieb Apr 12, 2023

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 Apr 12, 2023

doc fixes

dc19dc2

This comment has been minimized.

Sign in to view

Implement Send/Sync for CopyTaggedPtr

c155d51

rust-cloud-vms bot force-pushed the bless_tagged_pointers branch from f9cd8b5 to c155d51 Compare April 13, 2023 16:51

Noratrieb requested changes Apr 13, 2023

View reviewed changes

WaffleLapkin 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 Apr 13, 2023

scottmcm reviewed Apr 14, 2023

View reviewed changes

compiler/rustc_data_structures/src/aligned.rs Outdated Show resolved Hide resolved

scottmcm reviewed Apr 14, 2023

View reviewed changes

compiler/rustc_data_structures/src/tagged_ptr.rs Show resolved Hide resolved

scottmcm reviewed Apr 14, 2023

View reviewed changes

Noratrieb reviewed Apr 14, 2023

View reviewed changes

compiler/rustc_data_structures/src/tagged_ptr/copy/tests.rs Outdated Show resolved Hide resolved

compiler/rustc_data_structures/src/tagged_ptr/copy.rs Show resolved Hide resolved

WaffleLapkin mentioned this pull request Apr 14, 2023

Rewrite pointer::with_addr implementation #110318

Closed

This comment has been minimized.

Sign in to view

WaffleLapkin removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2023

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 15, 2023

Noratrieb approved these changes Apr 17, 2023

View reviewed changes

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 17, 2023

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

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 Apr 17, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 17, 2023

bors merged commit 7908a1d into rust-lang:master Apr 17, 2023

rustbot added this to the 1.71.0 milestone Apr 17, 2023

bors mentioned this pull request Apr 18, 2023

Introduce DynSend and DynSync auto trait for parallel compiler #107586

Merged

WaffleLapkin deleted the bless_tagged_pointers🙏 branch April 18, 2023 18:40

HKalbasi mentioned this pull request Dec 19, 2023

[strict provenance] make the compiler's tagged_ptr stuff conform #95493

Closed

WaffleLapkin mentioned this pull request Dec 22, 2023

[strict provenance] make compiler's Term and GenericArg conform #119217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tagged pointers, now with strict provenance! #110243

Tagged pointers, now with strict provenance! #110243

WaffleLapkin commented Apr 12, 2023

This comment has been minimized.

Noratrieb left a comment

Noratrieb Apr 13, 2023

Noratrieb Apr 13, 2023

scottmcm Apr 14, 2023

bors commented Apr 14, 2023

bors commented Apr 14, 2023

This comment has been minimized.

rust-timer commented Apr 15, 2023

Noratrieb commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

Noratrieb commented Apr 17, 2023

rust-log-analyzer commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

rust-timer commented Apr 18, 2023

Tagged pointers, now with strict provenance! #110243

Tagged pointers, now with strict provenance! #110243

Conversation

WaffleLapkin commented Apr 12, 2023

This comment has been minimized.

Noratrieb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 14, 2023

bors commented Apr 14, 2023

This comment has been minimized.

rust-timer commented Apr 15, 2023

Overall result: ❌ regressions - no action needed

Noratrieb commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

Noratrieb commented Apr 17, 2023

rust-log-analyzer commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

rust-timer commented Apr 18, 2023

Overall result: ❌✅ regressions and improvements - no action needed