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

tweak inttoptr allocation behavior #795

Merged
merged 4 commits into from
Jun 29, 2019
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jun 28, 2019

  • Make align_addr not offset by align for no reason.
  • Add some random slack between allocations to give them the chance to not be aligned.

Cc @christianpoveda

Fixes #791

src/intptrcast.rs Outdated Show resolved Hide resolved
@pvdrz
Copy link
Contributor

pvdrz commented Jun 28, 2019

Besides being picky about the residue computation. I was thinking about having the option to disable adding slack just from a performance POV. But I suppose that's a whole discussion for other day

@RalfJung
Copy link
Member Author

But I suppose that's a whole discussion for other day

Indeed. And that discussion would start with some benchmarks showing that adding slack even makes a measurable performance impact. Given how slow Miri is, I would be very surprised if it did. ;)


let base_addr = match alloc.extra.intptrcast.base_addr.get() {
Some(base_addr) => base_addr,
None => {
// This allocation does not have a base address yet, pick one.
let base_addr = Self::align_addr(global_state.next_base_addr, alloc.align.bytes());
global_state.next_base_addr = base_addr + alloc.bytes.len() as u64;
// Leave some space to the previous allocation, to give it some chance to be less aligned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why random and not maximally unaligned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be deterministic. Then I would be sure, for example, that by adding 2 to the base address of a 2-aligned allocation, I would get something 4-aligned.

@bors
Copy link
Contributor

bors commented Jun 29, 2019

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

RalfJung added 4 commits June 29, 2019 14:31
- Make `align_addr` not offset by `align` for no reason.
- Add some random slack between allocations to give them the chance to not be aligned.
@RalfJung
Copy link
Member Author

@bors r+

@bors
Copy link
Contributor

bors commented Jun 29, 2019

📌 Commit 0bb50ad has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jun 29, 2019

⌛ Testing commit 0bb50ad with merge 1522a47...

bors added a commit that referenced this pull request Jun 29, 2019
tweak inttoptr allocation behavior

- Make `align_addr` not offset by `align` for no reason.
- Add some random slack between allocations to give them the chance to not be aligned.

Cc @christianpoveda

Fixes #791
@bors
Copy link
Contributor

bors commented Jun 29, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 1522a47 to master...

@bors bors merged commit 0bb50ad into rust-lang:master Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addresses are unintentionally aligned
4 participants