Skip to content

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

New issue

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

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

Already on GitHub? # to your account

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
Collaborator

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
# for free to join this conversation on GitHub. Already have an account? # 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