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

miri: explain why we use static alignment in ref-to-place conversion #58615

Merged
merged 2 commits into from
Feb 23, 2019

Conversation

RalfJung
Copy link
Member

@eddyb @oli-obk do you think this makes sense? Or should we use the run-time alignment (align_of_val)? I am a bit worried about custom DSTs, but that affects way more areas of Miri so I'd ignore them for now.

r? @oli-obk

@RalfJung RalfJung changed the title explain why we use static alignment in ref-to-place conversion miri: explain why we use static alignment in ref-to-place conversion Feb 21, 2019
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 21, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Feb 21, 2019

Since the run-time alignment is stricter than the static alignment, we should probably not throw it away. Won't miri fail to detect certain unaligned reads otherwise?

@RalfJung
Copy link
Member Author

RalfJung commented Feb 21, 2019

Well, the question is whether a read must be aligned up to the dynamic alignment or whether the static alignment is sufficient.

I don't think we'll want to execute arbitrary code here, once align_of_val becomes user-definable. That seems like a strong argument to me to say that only the static alignment is actually required.

Only requiring static alignment is also consistent with our LLVM codegen, I believe.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 21, 2019

Oh, I misunderstood what was going on. I can't think of any reason the static alignment would ever not be sufficient. Manual alignment tricks basically need this to work anyway, so...

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 21, 2019

📌 Commit b01f81b has been approved by oli-obk

@bors 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 Feb 21, 2019
@RalfJung
Copy link
Member Author

I don't think you misunderstood -- this does mean Miri will miss cases where a read is aligned enough for the static alignment of a type, but not aligned enough for the run-time alignment of a type. But what I said above is essentially that we should not make such cases UB.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 21, 2019

Yes, if we made that UB, then turning a *const u32 into a *const u8 and reading through that would be UB, which is probably something done in embedded when starting out with u32s for aligned registers.

@RalfJung
Copy link
Member Author

Yes, if we made that UB, then turning a *const u32 into a *const u8 and reading through that would be UB

I have no idea what you are talking about... but no, it would not.

But with a type like

struct Foo<T: ?Sized> {
  a: u32,
  b: T
}

if we create a x: Foo<dyn Debug> from a Foo<u64>, the dynamic alignment required when reading x.a is 8, but currently we only require it to be 4. I am saying that is okay, i.e., it should NOT be UB to read x.a at alignment 4 even if the vtable says that there's a u64 in the b.

With x: Foo<u64>, reading x.a at alignment 4 is UB.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 21, 2019

With x: Foo<u64>, reading x.a at alignment 4 is UB.

wait what? that seems to immediately contradict

it should NOT be UB to read x.a at alignment 4

@RalfJung
Copy link
Member Author

RalfJung commented Feb 21, 2019

No, where is the contradiction? With y: Foo<u64>, when evaluating the place y.a, we start by evaluating y which must have alignment 8 (based on the layout of Foo<u64>). Then we adjust to the field's offset, adjusting 8 to offset 0 remains at 8, so the access happens with alignment 8. (When evaluating a projection, we don't care about the alignment of the field's type! We only care about the alignment we already know, and the offset.)

But with Foo<dyn Debug>, to determine the alignment 8 we'd have to use align_of_val. I don't think we want to do that. Instead we use layout.align.abi, which for this type is 4.

Centril added a commit to Centril/rust that referenced this pull request Feb 22, 2019
…oli-obk

miri: explain why we use static alignment in ref-to-place conversion

@eddyb @oli-obk do you think this makes sense? Or should we use the run-time alignment (`align_of_val`)? I am a bit worried about custom DSTs, but that affects way more areas of Miri so I'd ignore them for now.

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Feb 22, 2019
…oli-obk

miri: explain why we use static alignment in ref-to-place conversion

@eddyb @oli-obk do you think this makes sense? Or should we use the run-time alignment (`align_of_val`)? I am a bit worried about custom DSTs, but that affects way more areas of Miri so I'd ignore them for now.

r? @oli-obk
bors added a commit that referenced this pull request Feb 22, 2019
Rollup of 17 pull requests

Successful merges:

 - #57656 (Deprecate the unstable Vec::resize_default)
 - #58059 (deprecate before_exec in favor of unsafe pre_exec)
 - #58064 (override `VecDeque::try_rfold`, also update iterator)
 - #58198 (Suggest removing parentheses surrounding lifetimes)
 - #58431 (fix overlapping references in BTree)
 - #58555 (Add a note about 2018e if someone uses `try {` in 2015e)
 - #58588 (remove a bit of dead code)
 - #58589 (cleanup macro after 2018 transition)
 - #58591 (Dedup a rustdoc diagnostic construction)
 - #58600 (fix small documentation typo)
 - #58601 (Search for target_triple.json only if builtin target not found)
 - #58606 (Docs: put Future trait into spotlight)
 - #58607 (Fixes #58586: Make E0505 erronous example fail for the 2018 edition)
 - #58615 (miri: explain why we use static alignment in ref-to-place conversion)
 - #58620 (introduce benchmarks of BTreeSet.intersection)
 - #58621 (Update miri links)
 - #58632 (Make std feature list sorted)

Failed merges:

r? @ghost
@bors bors merged commit b01f81b into rust-lang:master Feb 23, 2019
@RalfJung RalfJung deleted the ref-to-place-alignment branch June 10, 2019 11:04
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.

4 participants