Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve reallocation in alloc_system on Windows #42331

Merged
merged 2 commits into from
Jun 3, 2017

Conversation

retep998
Copy link
Member

@retep998 retep998 commented May 31, 2017

For allocations where the alignment is greater than the alignment guaranteed by HeapAlloc, the implementation will overallocate by align bytes. align_ptr will the get the first address within that allocation satisfying the alignment (other than the base of the allocation) and write the address of the original allocation a pointer size before that aligned address. What the offset is within any given overaligned allocation varies on a per allocation basis, which means the offset of data written to the allocation varies on a per allocation basis.

The old version of reallocate would always call HeapRealloc, which will move the allocation to a new address, which has the side effect of causing the aligned offset within the allocation to possibly change. Since the data itself wasn't moved to the new offset but remained at its old offset, this effectively changed the data in the allocation which is of course quite bad.

The new version does exactly what the unix version does, which is to allocate a brand new buffer, figure out what the aligned address is, and then memcpy over the data from the old aligned address to the new aligned address, which works just fine.

While I was at it, I updated reallocate_inplace to be able to do inplace reallocations of overaligned data. Because inplace reallocation does not change the base address of the allocation, it does not change the aligned offset, therefore the data will safely remain in the same location without issue.

Fixes #42025

@alexcrichton
Copy link
Member

I'm not sure I quite understand what the bug is here and how this is fixing it, could you explain in a bit more detail? Would it be possible to add tests for this?

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 1, 2017
@retep998
Copy link
Member Author

retep998 commented Jun 1, 2017

Updated topic with details.

Here's an example that fails with the old implementation which I will turn into a test to ensure the new implementation is correct:

#![feature(attr_literals, repr_align)]

#[repr(align(256))]
struct Foo(usize);

fn main() {
    let mut foo = vec![Foo(273)];
    for i in 0..0x1000 {
        foo.reserve_exact(i);
        assert!(foo[0].0 == 273);
        assert!(foo.as_ptr() as usize & 0xff == 0);
        foo.shrink_to_fit();
        assert!(foo[0].0 == 273);
        assert!(foo.as_ptr() as usize & 0xff == 0);
    }
}

@retep998 retep998 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 1, 2017
@alexcrichton
Copy link
Member

@bors: r+

Makes sense to me, thanks!

@bors
Copy link
Contributor

bors commented Jun 1, 2017

📌 Commit 4be66b8 has been approved by alexcrichton

@retep998 retep998 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 Jun 1, 2017
@bors
Copy link
Contributor

bors commented Jun 1, 2017

⌛ Testing commit 4be66b8 with merge fd0fd18...

@bors
Copy link
Contributor

bors commented Jun 1, 2017

💔 Test failed - status-travis

@steveklabnik
Copy link
Member

@bors: retry

@Mark-Simulacrum
Copy link
Member

Spurious failure was #42118. Logging in spreadsheet.

@bors
Copy link
Contributor

bors commented Jun 2, 2017

☔ The latest upstream changes (presumably #41670) made this pull request unmergeable. Please resolve the merge conflicts.

@retep998 retep998 force-pushed the standard-relocation-coupon branch from 4be66b8 to 42ac311 Compare June 2, 2017 10:31
@eddyb
Copy link
Member

eddyb commented Jun 2, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jun 2, 2017

📌 Commit 42ac311 has been approved by alexcrichton

@gereeter
Copy link
Contributor

gereeter commented Jun 3, 2017

Would it be possible to copy that explanation of the change into the main issue body so that it gets immortalized in the commit history?

@bors
Copy link
Contributor

bors commented Jun 3, 2017

⌛ Testing commit 42ac311 with merge fbb9276...

bors added a commit that referenced this pull request Jun 3, 2017
…chton

Improve reallocation in alloc_system on Windows

Fixes #42025
@bors
Copy link
Contributor

bors commented Jun 3, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing fbb9276 to master...

@bors bors merged commit 42ac311 into rust-lang:master Jun 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants