Skip to content

Commit 2835ca6

Browse files
committed
Auto merge of #70174 - JohnTitor:rollup-0lum0jh, r=JohnTitor
Rollup of 9 pull requests Successful merges: - #69618 (Clarify the relationship between `forget()` and `ManuallyDrop`.) - #69768 (Compute the correct layout for variants of uninhabited enums) - #69935 (codegen/mir: support polymorphic `InstanceDef`s) - #70103 (Clean up E0437 explanation) - #70131 (Add regression test for TAIT lifetime inference (issue #55099)) - #70133 (remove unused imports) - #70145 (doc: Add quote to .init_array) - #70146 (Clean up e0438 explanation) - #70150 (triagebot.toml: accept cleanup-crew) Failed merges: r? @ghost
2 parents f4c675c + 43c7a50 commit 2835ca6

File tree

16 files changed

+210
-126
lines changed

16 files changed

+210
-126
lines changed

src/libcore/mem/mod.rs

+49-16
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,9 @@ pub use crate::intrinsics::transmute;
5858
///
5959
/// # Examples
6060
///
61-
/// Leak an I/O object, never closing the file:
61+
/// The canonical safe use of `mem::forget` is to circumvent a value's destructor
62+
/// implemented by the `Drop` trait. For example, this will leak a `File`, i.e. reclaim
63+
/// the space taken by the variable but never close the underlying system resource:
6264
///
6365
/// ```no_run
6466
/// use std::mem;
@@ -68,9 +70,40 @@ pub use crate::intrinsics::transmute;
6870
/// mem::forget(file);
6971
/// ```
7072
///
71-
/// The practical use cases for `forget` are rather specialized and mainly come
72-
/// up in unsafe or FFI code. However, [`ManuallyDrop`] is usually preferred
73-
/// for such cases, e.g.:
73+
/// This is useful when the ownership of the underlying resource was previously
74+
/// transferred to code outside of Rust, for example by transmitting the raw
75+
/// file descriptor to C code.
76+
///
77+
/// # Relationship with `ManuallyDrop`
78+
///
79+
/// While `mem::forget` can also be used to transfer *memory* ownership, doing so is error-prone.
80+
/// [`ManuallyDrop`] should be used instead. Consider, for example, this code:
81+
///
82+
/// ```
83+
/// use std::mem;
84+
///
85+
/// let mut v = vec![65, 122];
86+
/// // Build a `String` using the contents of `v`
87+
/// let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), v.len(), v.capacity()) };
88+
/// // leak `v` because its memory is now managed by `s`
89+
/// mem::forget(v); // ERROR - v is invalid and must not be passed to a function
90+
/// assert_eq!(s, "Az");
91+
/// // `s` is implicitly dropped and its memory deallocated.
92+
/// ```
93+
///
94+
/// There are two issues with the above example:
95+
///
96+
/// * If more code were added between the construction of `String` and the invocation of
97+
/// `mem::forget()`, a panic within it would cause a double free because the same memory
98+
/// is handled by both `v` and `s`.
99+
/// * After calling `v.as_mut_ptr()` and transmitting the ownership of the data to `s`,
100+
/// the `v` value is invalid. Even when a value is just moved to `mem::forget` (which won't
101+
/// inspect it), some types have strict requirements on their values that
102+
/// make them invalid when dangling or no longer owned. Using invalid values in any
103+
/// way, including passing them to or returning them from functions, constitutes
104+
/// undefined behavior and may break the assumptions made by the compiler.
105+
///
106+
/// Switching to `ManuallyDrop` avoids both issues:
74107
///
75108
/// ```
76109
/// use std::mem::ManuallyDrop;
@@ -80,24 +113,24 @@ pub use crate::intrinsics::transmute;
80113
/// // does not get dropped!
81114
/// let mut v = ManuallyDrop::new(v);
82115
/// // Now disassemble `v`. These operations cannot panic, so there cannot be a leak.
83-
/// let ptr = v.as_mut_ptr();
84-
/// let cap = v.capacity();
116+
/// let (ptr, len, cap) = (v.as_mut_ptr(), v.len(), v.capacity());
85117
/// // Finally, build a `String`.
86-
/// let s = unsafe { String::from_raw_parts(ptr, 2, cap) };
118+
/// let s = unsafe { String::from_raw_parts(ptr, len, cap) };
87119
/// assert_eq!(s, "Az");
88120
/// // `s` is implicitly dropped and its memory deallocated.
89121
/// ```
90122
///
91-
/// Using `ManuallyDrop` here has two advantages:
123+
/// `ManuallyDrop` robustly prevents double-free because we disable `v`'s destructor
124+
/// before doing anything else. `mem::forget()` doesn't allow this because it consumes its
125+
/// argument, forcing us to call it only after extracting anything we need from `v`. Even
126+
/// if a panic were introduced between construction of `ManuallyDrop` and building the
127+
/// string (which cannot happen in the code as shown), it would result in a leak and not a
128+
/// double free. In other words, `ManuallyDrop` errs on the side of leaking instead of
129+
/// erring on the side of (double-)dropping.
92130
///
93-
/// * We do not "touch" `v` after disassembling it. For some types, operations
94-
/// such as passing ownership (to a function like `mem::forget`) requires them to actually
95-
/// be fully owned right now; that is a promise we do not want to make here as we are
96-
/// in the process of transferring ownership to the new `String` we are building.
97-
/// * In case of an unexpected panic, `ManuallyDrop` is not dropped, but if the panic
98-
/// occurs before `mem::forget` was called we might end up dropping invalid data,
99-
/// or double-dropping. In other words, `ManuallyDrop` errs on the side of leaking
100-
/// instead of erring on the side of dropping.
131+
/// Also, `ManuallyDrop` prevents us from having to "touch" `v` after transferring the
132+
/// ownership to `s` - the final step of interacting with `v` to dispoe of it without
133+
/// running its destructor is entirely avoided.
101134
///
102135
/// [drop]: fn.drop.html
103136
/// [uninit]: fn.uninitialized.html

src/libpanic_unwind/hermit.rs

-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
55
use alloc::boxed::Box;
66
use core::any::Any;
7-
use core::ptr;
87

98
pub unsafe fn cleanup(_ptr: *mut u8) -> Box<dyn Any + Send> {
109
extern "C" {

src/librustc/ty/instance.rs

+26
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,32 @@ impl<'tcx> Instance<'tcx> {
381381
Instance { def, substs }
382382
}
383383

384+
/// FIXME(#69925) Depending on the kind of `InstanceDef`, the MIR body associated with an
385+
/// instance is expressed in terms of the generic parameters of `self.def_id()`, and in other
386+
/// cases the MIR body is expressed in terms of the types found in the substitution array.
387+
/// In the former case, we want to substitute those generic types and replace them with the
388+
/// values from the substs when monomorphizing the function body. But in the latter case, we
389+
/// don't want to do that substitution, since it has already been done effectively.
390+
///
391+
/// This function returns `Some(substs)` in the former case and None otherwise -- i.e., if
392+
/// this function returns `None`, then the MIR body does not require substitution during
393+
/// monomorphization.
394+
pub fn substs_for_mir_body(&self) -> Option<SubstsRef<'tcx>> {
395+
match self.def {
396+
InstanceDef::CloneShim(..)
397+
| InstanceDef::DropGlue(_, Some(_)) => None,
398+
InstanceDef::ClosureOnceShim { .. }
399+
| InstanceDef::DropGlue(..)
400+
// FIXME(#69925): `FnPtrShim` should be in the other branch.
401+
| InstanceDef::FnPtrShim(..)
402+
| InstanceDef::Item(_)
403+
| InstanceDef::Intrinsic(..)
404+
| InstanceDef::ReifyShim(..)
405+
| InstanceDef::Virtual(..)
406+
| InstanceDef::VtableShim(..) => Some(self.substs),
407+
}
408+
}
409+
384410
pub fn is_vtable_shim(&self) -> bool {
385411
if let InstanceDef::VtableShim(..) = self.def { true } else { false }
386412
}

src/librustc/ty/layout.rs

+11-3
Original file line numberDiff line numberDiff line change
@@ -780,8 +780,8 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
780780
present_first @ Some(_) => present_first,
781781
// Uninhabited because it has no variants, or only absent ones.
782782
None if def.is_enum() => return tcx.layout_raw(param_env.and(tcx.types.never)),
783-
// if it's a struct, still compute a layout so that we can still compute the
784-
// field offsets
783+
// If it's a struct, still compute a layout so that we can still compute the
784+
// field offsets.
785785
None => Some(VariantIdx::new(0)),
786786
};
787787

@@ -1987,7 +1987,15 @@ where
19871987
{
19881988
fn for_variant(this: TyLayout<'tcx>, cx: &C, variant_index: VariantIdx) -> TyLayout<'tcx> {
19891989
let details = match this.variants {
1990-
Variants::Single { index } if index == variant_index => this.details,
1990+
Variants::Single { index }
1991+
// If all variants but one are uninhabited, the variant layout is the enum layout.
1992+
if index == variant_index &&
1993+
// Don't confuse variants of uninhabited enums with the enum itself.
1994+
// For more details see https://github.com/rust-lang/rust/issues/69763.
1995+
this.fields != FieldPlacement::Union(0) =>
1996+
{
1997+
this.details
1998+
}
19911999

19922000
Variants::Single { index } => {
19932001
// Deny calling for_variant more than once for non-Single enums.

src/librustc_codegen_ssa/mir/mod.rs

+11-6
Original file line numberDiff line numberDiff line change
@@ -86,13 +86,18 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {
8686
impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
8787
pub fn monomorphize<T>(&self, value: &T) -> T
8888
where
89-
T: TypeFoldable<'tcx>,
89+
T: Copy + TypeFoldable<'tcx>,
9090
{
91-
self.cx.tcx().subst_and_normalize_erasing_regions(
92-
self.instance.substs,
93-
ty::ParamEnv::reveal_all(),
94-
value,
95-
)
91+
debug!("monomorphize: self.instance={:?}", self.instance);
92+
if let Some(substs) = self.instance.substs_for_mir_body() {
93+
self.cx.tcx().subst_and_normalize_erasing_regions(
94+
substs,
95+
ty::ParamEnv::reveal_all(),
96+
&value,
97+
)
98+
} else {
99+
self.cx.tcx().normalize_erasing_regions(ty::ParamEnv::reveal_all(), *value)
100+
}
96101
}
97102
}
98103

src/librustc_error_codes/error_codes/E0437.md

+5-4
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
1-
Trait implementations can only implement associated types that are members of
2-
the trait in question. This error indicates that you attempted to implement
3-
an associated type whose name does not match the name of any associated type
4-
in the trait.
1+
An associated type whose name does not match any of the associated types
2+
in the trait was used when implementing the trait.
53

64
Erroneous code example:
75

@@ -13,6 +11,9 @@ impl Foo for i32 {
1311
}
1412
```
1513

14+
Trait implementations can only implement associated types that are members of
15+
the trait in question.
16+
1617
The solution to this problem is to remove the extraneous associated type:
1718

1819
```

src/librustc_error_codes/error_codes/E0438.md

+5-4
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
1-
Trait implementations can only implement associated constants that are
2-
members of the trait in question. This error indicates that you
3-
attempted to implement an associated constant whose name does not
4-
match the name of any associated constant in the trait.
1+
An associated constant whose name does not match any of the associated constants
2+
in the trait was used when implementing the trait.
53

64
Erroneous code example:
75

@@ -13,6 +11,9 @@ impl Foo for i32 {
1311
}
1412
```
1513

14+
Trait implementations can only implement associated constants that are
15+
members of the trait in question.
16+
1617
The solution to this problem is to remove the extraneous associated constant:
1718

1819
```

src/librustc_mir/interpret/eval_context.rs

+17-10
Original file line numberDiff line numberDiff line change
@@ -335,15 +335,25 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
335335

336336
/// Call this on things you got out of the MIR (so it is as generic as the current
337337
/// stack frame), to bring it into the proper environment for this interpreter.
338+
pub(super) fn subst_from_current_frame_and_normalize_erasing_regions<T: TypeFoldable<'tcx>>(
339+
&self,
340+
value: T,
341+
) -> T {
342+
self.subst_from_frame_and_normalize_erasing_regions(self.frame(), value)
343+
}
344+
345+
/// Call this on things you got out of the MIR (so it is as generic as the provided
346+
/// stack frame), to bring it into the proper environment for this interpreter.
338347
pub(super) fn subst_from_frame_and_normalize_erasing_regions<T: TypeFoldable<'tcx>>(
339348
&self,
349+
frame: &Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
340350
value: T,
341351
) -> T {
342-
self.tcx.subst_and_normalize_erasing_regions(
343-
self.frame().instance.substs,
344-
self.param_env,
345-
&value,
346-
)
352+
if let Some(substs) = frame.instance.substs_for_mir_body() {
353+
self.tcx.subst_and_normalize_erasing_regions(substs, self.param_env, &value)
354+
} else {
355+
self.tcx.normalize_erasing_regions(self.param_env, value)
356+
}
347357
}
348358

349359
/// The `substs` are assumed to already be in our interpreter "universe" (param_env).
@@ -371,11 +381,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
371381
None => {
372382
let layout = crate::interpret::operand::from_known_layout(layout, || {
373383
let local_ty = frame.body.local_decls[local].ty;
374-
let local_ty = self.tcx.subst_and_normalize_erasing_regions(
375-
frame.instance.substs,
376-
self.param_env,
377-
&local_ty,
378-
);
384+
let local_ty =
385+
self.subst_from_frame_and_normalize_erasing_regions(frame, local_ty);
379386
self.layout_of(local_ty)
380387
})?;
381388
if let Some(state) = frame.locals.get(local) {

src/librustc_mir/interpret/operand.rs

+3-2
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
355355
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
356356
let base = match op.try_as_mplace(self) {
357357
Ok(mplace) => {
358-
// The easy case
358+
// We can reuse the mplace field computation logic for indirect operands.
359359
let field = self.mplace_field(mplace, field)?;
360360
return Ok(field.into());
361361
}
@@ -490,7 +490,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
490490
Copy(ref place) | Move(ref place) => self.eval_place_to_op(place, layout)?,
491491

492492
Constant(ref constant) => {
493-
let val = self.subst_from_frame_and_normalize_erasing_regions(constant.literal);
493+
let val =
494+
self.subst_from_current_frame_and_normalize_erasing_regions(constant.literal);
494495
self.eval_const_to_op(val, layout)?
495496
}
496497
};

src/librustc_mir/interpret/place.rs

+5-11
Original file line numberDiff line numberDiff line change
@@ -410,14 +410,6 @@ where
410410
stride * field
411411
}
412412
layout::FieldPlacement::Union(count) => {
413-
// This is a narrow bug-fix for rust-lang/rust#69191: if we are
414-
// trying to access absent field of uninhabited variant, then
415-
// signal UB (but don't ICE the compiler).
416-
// FIXME temporary hack to work around incoherence between
417-
// layout computation and MIR building
418-
if field >= count as u64 && base.layout.abi == layout::Abi::Uninhabited {
419-
throw_ub!(Unreachable);
420-
}
421413
assert!(
422414
field < count as u64,
423415
"Tried to access field {} of union {:#?} with {} fields",
@@ -648,9 +640,11 @@ where
648640
// bail out.
649641
None => Place::null(&*self),
650642
},
651-
layout: self.layout_of(self.subst_from_frame_and_normalize_erasing_regions(
652-
self.frame().body.return_ty(),
653-
))?,
643+
layout: self.layout_of(
644+
self.subst_from_current_frame_and_normalize_erasing_regions(
645+
self.frame().body.return_ty(),
646+
),
647+
)?,
654648
}
655649
}
656650
local => PlaceTy {

src/librustc_mir/interpret/step.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
248248
}
249249

250250
NullaryOp(mir::NullOp::SizeOf, ty) => {
251-
let ty = self.subst_from_frame_and_normalize_erasing_regions(ty);
251+
let ty = self.subst_from_current_frame_and_normalize_erasing_regions(ty);
252252
let layout = self.layout_of(ty)?;
253253
assert!(
254254
!layout.is_unsized(),

0 commit comments

Comments
 (0)