Attach range metadata to alignment loads from vtables #91569

erikdesjardins · 2021-12-05T21:36:03Z

...because alignment is always nonzero[0].

This helps eliminate redundant runtime alignment checks, when a DST
is a field of a struct whose remaining fields have alignment 1.

Fixes #91438.

[0]:

The reference says that alignment must be at least 1.

And in practice, the alignment field for all vtables is generated here:

rust/compiler/rustc_middle/src/ty/vtable.rs

Lines 68 to 90 in 772d51f

    
           let align = layout.align.abi.bytes(); 
        
           let ptr_size = tcx.data_layout.pointer_size; 
        
           let ptr_align = tcx.data_layout.pointer_align.abi; 
        
           let vtable_size = ptr_size * u64::try_from(vtable_entries.len()).unwrap(); 
        
           let mut vtable = Allocation::uninit(vtable_size, ptr_align, /* panic_on_fail */ true).unwrap(); 
        
           // No need to do any alignment checks on the memory accesses below, because we know the 
        
           // allocation is correctly aligned as we created it above. Also we're only offsetting by 
        
           // multiples of `ptr_align`, which means that it will stay aligned to `ptr_align`. 
        
           for (idx, entry) in vtable_entries.iter().enumerate() { 
        
               let idx: u64 = u64::try_from(idx).unwrap(); 
        
               let scalar = match entry { 
        
                   VtblEntry::MetadataDropInPlace => { 
        
                       let instance = ty::Instance::resolve_drop_in_place(tcx, ty); 
        
                       let fn_alloc_id = tcx.create_fn_alloc(instance); 
        
                       let fn_ptr = Pointer::from(fn_alloc_id); 
        
                       ScalarMaybeUninit::from_pointer(fn_ptr, &tcx) 
        
                   } 
        
                   VtblEntry::MetadataSize => Scalar::from_uint(size, ptr_size).into(), 
        
                   VtblEntry::MetadataAlign => Scalar::from_uint(align, ptr_size).into(),

and is nonzero because Align::bytes() is always nonzero.

...because alignment is always nonzero. This helps eliminate redundant runtime alignment checks, when a DST is a field of a struct whose remaining fields have alignment 1.

rust-highfive · 2021-12-05T21:36:06Z

r? @michaelwoerister

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

Aaron1011 · 2021-12-06T00:47:38Z

Since the range metadata is only being applied to the actual load from the vtable, I believe that this is fine. Regardless of what other decisions we make about pointer metadata/vtable validity invariants, the vtable clearly needs to be valid at the point where we actually read from it. cc @rust-lang/wg-unsafe-code-guidelines

RalfJung · 2021-12-06T01:31:57Z

Yeah, this sounds like a reasonable requirement to impose on vtables.

Miri already considers invalid alignments to be UB:

rust/compiler/rustc_const_eval/src/interpret/traits.rs

Line 117 in b7cc991

    
           let align = Align::from_bytes(align).map_err(|e| err_ub!(InvalidVtableAlignment(e)))?;

michaelwoerister · 2021-12-06T10:58:05Z

r? @cuviper (or maybe someone else from @rust-lang/wg-llvm)

nikic · 2021-12-06T20:03:25Z

Looks reasonable to me. I guess we could instead special case alignment 1 instead (in which case we can always pick the other), but using range metadata does seem more elegant/general.

@bors r+ rollup=never (may have optimization effect)

bors · 2021-12-06T20:03:27Z

📌 Commit 2ff5a3e has been approved by nikic

bors · 2021-12-08T14:28:35Z

⌛ Testing commit 2ff5a3e with merge 8323fd5e21e152f70d521ca877edcf143a05df74...

bors · 2021-12-08T14:54:37Z

💔 Test failed - checks-actions

src/test/codegen/dst-vtable-align-nonzero.rs

nikic · 2021-12-09T08:01:09Z

@bors r+

bors · 2021-12-09T08:01:11Z

📌 Commit 94f0833 has been approved by nikic

bors · 2021-12-09T12:57:33Z

⌛ Testing commit 94f0833 with merge 5099869c742d325347de01576a17fbe708a534d3...

rust-log-analyzer · 2021-12-09T15:01:07Z

The job dist-x86_64-apple failed! Check out the build log: (web) (plain)

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

bors · 2021-12-09T15:01:36Z

💔 Test failed - checks-actions

nikic · 2021-12-09T19:59:43Z

2021-12-09T15:00:42.0266900Z command did not execute successfully: "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/stage0-tools-bin/unstable-book-gen" "/Users/runner/work/rust/rust/library" "/Users/runner/work/rust/rust/compiler" "/Users/runner/work/rust/rust/src" "/Users/runner/work/rust/rust/build/x86_64-apple-darwin/md-doc/unstable-book"
2021-12-09T15:00:42.0268820Z expected success, got: signal: 9

Not really clear what happened here, but probably spurious...

@bors retry

bors · 2021-12-13T04:29:29Z

⌛ Testing commit 94f0833 with merge 4a7fb97...

bors · 2021-12-13T07:30:32Z

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 4a7fb97 to master...

rust-timer · 2021-12-13T09:01:00Z

Finished benchmarking commit (4a7fb97): comparison url.

Summary: This change led to large relevant improvements 🎉 in compiler performance.

Large improvement in instruction counts (up to -3.8% on incr-unchanged builds of deep-vector)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Add 0..=isize::MAX range metadata to size loads from vtables This is the (much belated) size counterpart to rust-lang#91569. Inspired by https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/Range.20metadata.20for.20.60size_of_val.60.20and.20other.20isize.3A.3AMAX.20limits. This could help optimize layout computations based on the size of a dyn trait. Though, admittedly, adding this to vtables wouldn't be as beneficial as adding it to slice len, which is used much more often. Miri detects this UB already: https://github.com/rust-lang/rust/blob/b7cc99142ad0cfe47e2fe9f7a82eaf5b672c0573/compiler/rustc_const_eval/src/interpret/traits.rs#L119-L121 (In fact Miri goes further, [assuming a 48-bit address space on 64-bit platforms](https://github.com/rust-lang/rust/blob/9db224fc908059986c179fc6ec433944e9cfce50/compiler/rustc_abi/src/lib.rs#L312-L331), but I don't think we can assume that in an optimization.)

Attach range metadata to alignment loads from vtables

2ff5a3e

...because alignment is always nonzero. This helps eliminate redundant runtime alignment checks, when a DST is a field of a struct whose remaining fields have alignment 1.

rust-highfive assigned michaelwoerister Dec 5, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2021

erikdesjardins mentioned this pull request Dec 5, 2021

Potentially redundant check for zero alignment in generated assembly #91438

Closed

rust-highfive assigned cuviper and unassigned michaelwoerister Dec 6, 2021

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 Dec 6, 2021

This comment has been minimized.

Sign in to view

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 Dec 8, 2021

nikic reviewed Dec 8, 2021

View reviewed changes

src/test/codegen/dst-vtable-align-nonzero.rs Show resolved Hide resolved

add // compile-flags: -O to test that depends on opts

94f0833

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 Dec 9, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 9, 2021

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 Dec 9, 2021

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 Dec 9, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2021

bors merged commit 4a7fb97 into rust-lang:master Dec 13, 2021

rustbot added this to the 1.59.0 milestone Dec 13, 2021

erikdesjardins deleted the vt-align branch December 14, 2021 00:06

erikdesjardins mentioned this pull request Dec 8, 2022

Add 0..=isize::MAX range metadata to size loads from vtables #105446

Merged

	let align = layout.align.abi.bytes();

	let ptr_size = tcx.data_layout.pointer_size;
	let ptr_align = tcx.data_layout.pointer_align.abi;

	let vtable_size = ptr_size * u64::try_from(vtable_entries.len()).unwrap();
	let mut vtable = Allocation::uninit(vtable_size, ptr_align, /* panic_on_fail */ true).unwrap();

	// No need to do any alignment checks on the memory accesses below, because we know the
	// allocation is correctly aligned as we created it above. Also we're only offsetting by
	// multiples of `ptr_align`, which means that it will stay aligned to `ptr_align`.

	for (idx, entry) in vtable_entries.iter().enumerate() {
	let idx: u64 = u64::try_from(idx).unwrap();
	let scalar = match entry {
	VtblEntry::MetadataDropInPlace => {
	let instance = ty::Instance::resolve_drop_in_place(tcx, ty);
	let fn_alloc_id = tcx.create_fn_alloc(instance);
	let fn_ptr = Pointer::from(fn_alloc_id);
	ScalarMaybeUninit::from_pointer(fn_ptr, &tcx)
	}
	VtblEntry::MetadataSize => Scalar::from_uint(size, ptr_size).into(),
	VtblEntry::MetadataAlign => Scalar::from_uint(align, ptr_size).into(),

Attach range metadata to alignment loads from vtables #91569

Attach range metadata to alignment loads from vtables #91569

Uh oh!

Conversation

erikdesjardins commented Dec 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 5, 2021

Uh oh!

Aaron1011 commented Dec 6, 2021

Uh oh!

RalfJung commented Dec 6, 2021

Uh oh!

michaelwoerister commented Dec 6, 2021

Uh oh!

nikic commented Dec 6, 2021

Uh oh!

bors commented Dec 6, 2021

Uh oh!

bors commented Dec 8, 2021

Uh oh!

This comment has been minimized.

bors commented Dec 8, 2021

Uh oh!

Uh oh!

nikic commented Dec 9, 2021

Uh oh!

bors commented Dec 9, 2021

Uh oh!

bors commented Dec 9, 2021

Uh oh!

rust-log-analyzer commented Dec 9, 2021

Uh oh!

bors commented Dec 9, 2021

Uh oh!

nikic commented Dec 9, 2021

Uh oh!

bors commented Dec 13, 2021

Uh oh!

bors commented Dec 13, 2021

Uh oh!

rust-timer commented Dec 13, 2021

Uh oh!

Uh oh!

erikdesjardins commented Dec 5, 2021 •

edited

Loading