Adjust the pointer to an unsized field to the correct alignment #30245
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -742,9 +742,58 @@ fn trans_field<'blk, 'tcx, F>(bcx: Block<'blk, 'tcx>, | |||
| if type_is_sized(bcx.tcx(), d.ty) { | |||
| DatumBlock { datum: d.to_expr_datum(), bcx: bcx } | |||
| } else { | |||
| let info = Load(bcx, get_meta(bcx, base_datum.val)); | |||
| // Adjust the pointer, if needed, so it has the correct alignment. | |||
| match d.ty.sty { | |||
|
lgtm, r+ with the extra test and nits addressed. |
|
Bah, I just realised this doesn't handle pattern matching! Looks like I need to re-think this some more. |
Indeed; I might prefer to see the code, when compiled in debug mode, emit a debugtrap upon encountering zero, rather than trying to accept it. (I'm more concerned about catching bugs elsewhere than I am about the potential perfomance overhead of handling zero.) |
DST fields, being of an unknown type, are not automatically aligned properly, so a pointer to the field needs to be aligned using the information in the vtable. Fixes rust-lang#26403 and a number of other DST-related bugs discovered while implementing this.
Aatch
force-pushed
the
dynamic-align-dst
branch
from
e13f0c2
to
a2557d4
Compare
|
Basically rewrote the entire change, so I squashed it down into a single commit. The test has been amended to include a test for getting pointer to a DST field via pattern matching, something that caused an ICE previously. |
| @@ -116,8 +116,14 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> { | |||
| LvalueTy::Downcast { adt_def: _, substs: _, variant_index: v } => v, | |||
| }; | |||
| let discr = discr as u64; | |||
| (adt::trans_field_ptr(bcx, &base_repr, tr_base.llval, discr, field.index()), | |||
| if common::type_is_sized(tcx, projected_ty.to_ty(tcx)) { | |||
| let is_sized = common::type_is_sized(tcx, projected_ty.to_ty(tcx)); | |||
There was a problem hiding this comment.
Just use the MaybeSizedValue::unsized_ directly. llextra has the right semantics.
There was a problem hiding this comment.
As in llextra is a nullptr if the type is sized? Works for me. I wasn't sure because of the if type_is_sized { llextra } else { ptr::null_mut() } in there.
|
I'm pretty sure I broke something related to slice DSTs, not sure how though. I'm get the same failures locally as the travis build. |
The presence of the drop flag caused the offset calculation to be incorrect, leading to the pointer being incorrect. This has been fixed by calculating the offset based on the field index (and not assuming that the field is always the last one). However, I've also stopped the drop flag from being added to the end of unsized structs to begin with. Since it's not actually accessed for unsized structs, and isn't actually where we would say it is, this made more sense.
Aatch
force-pushed
the
dynamic-align-dst
branch
from
aceb9c8
to
d6eb063
Compare
Fixes #26403 This adjusts the pointer, if needed, to the correct alignment by using the alignment information in the vtable. Handling zero might not be necessary, as it shouldn't actually occur. I've left it as it's own commit so it can be removed fairly easily if people don't think it's worth doing. The way it's handled though means that there shouldn't be much impact on performance.
Fixes #26403
This adjusts the pointer, if needed, to the correct alignment by using the alignment information in the vtable.
Handling zero might not be necessary, as it shouldn't actually occur. I've left it as it's own commit so it can be removed fairly easily if people don't think it's worth doing. The way it's handled though means that there shouldn't be much impact on performance.