Skip to content

Commit

Permalink
Auto merge of #104862 - saethlin:mir-niche-checks, r=<try>
Browse files Browse the repository at this point in the history
Check for occupied niches

Implementation of rust-lang/compiler-team#624

Crater run has 62 crates that hit the check, 43 of those are published to crates.io. I see a lot of null function pointers and use of `mem::uninitialized` where the 0x1-filling collides with an enum niche.

But that is with full niche checks; checking transmute, plus any place where that we Copy, Move, or Inspect. Such checking is definitely too thorough to be on by default because it is 2x compile time overhead.

---

During implementation, this ran into llvm/llvm-project#68381

r? `@ghost`
  • Loading branch information
bors committed Nov 10, 2023
2 parents d4c86cf + 5c19cfd commit eaa4669
Show file tree
Hide file tree
Showing 37 changed files with 710 additions and 51 deletions.
33 changes: 29 additions & 4 deletions compiler/rustc_codegen_cranelift/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use rustc_index::IndexVec;
use rustc_middle::ty::adjustment::PointerCoercion;
use rustc_middle::ty::layout::FnAbiOf;
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::GenericArg;

use crate::constant::ConstantCx;
use crate::debuginfo::FunctionDebugContext;
Expand Down Expand Up @@ -354,6 +355,7 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) {
rustc_hir::LangItem::PanicBoundsCheck,
&[index, len, location],
source_info.span,
None,
);
}
AssertKind::MisalignedPointerDereference { ref required, ref found } => {
Expand All @@ -366,8 +368,25 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) {
rustc_hir::LangItem::PanicMisalignedPointerDereference,
&[required, found, location],
source_info.span,
None,
);
}
AssertKind::OccupiedNiche { ref found, ref start, ref end } => {
let found = codegen_operand(fx, found);
let generic_arg = ty::GenericArg::from(found.layout().ty);
let found = found.load_scalar(fx);
let start = codegen_operand(fx, start).load_scalar(fx);
let end = codegen_operand(fx, end).load_scalar(fx);
let location = fx.get_caller_location(source_info).load_scalar(fx);

codegen_panic_inner(
fx,
rustc_hir::LangItem::PanicOccupiedNiche,
&[found, start, end, location],
source_info.span,
Some(generic_arg),
)
}
_ => {
let msg_str = msg.description();
codegen_panic(fx, msg_str, source_info);
Expand Down Expand Up @@ -945,7 +964,7 @@ pub(crate) fn codegen_panic<'tcx>(
let msg_len = fx.bcx.ins().iconst(fx.pointer_type, i64::try_from(msg_str.len()).unwrap());
let args = [msg_ptr, msg_len, location];

codegen_panic_inner(fx, rustc_hir::LangItem::Panic, &args, source_info.span);
codegen_panic_inner(fx, rustc_hir::LangItem::Panic, &args, source_info.span, None);
}

pub(crate) fn codegen_panic_nounwind<'tcx>(
Expand All @@ -957,7 +976,7 @@ pub(crate) fn codegen_panic_nounwind<'tcx>(
let msg_len = fx.bcx.ins().iconst(fx.pointer_type, i64::try_from(msg_str.len()).unwrap());
let args = [msg_ptr, msg_len];

codegen_panic_inner(fx, rustc_hir::LangItem::PanicNounwind, &args, source_info.span);
codegen_panic_inner(fx, rustc_hir::LangItem::PanicNounwind, &args, source_info.span, None);
}

pub(crate) fn codegen_unwind_terminate<'tcx>(
Expand All @@ -967,18 +986,24 @@ pub(crate) fn codegen_unwind_terminate<'tcx>(
) {
let args = [];

codegen_panic_inner(fx, reason.lang_item(), &args, source_info.span);
codegen_panic_inner(fx, reason.lang_item(), &args, source_info.span, None);
}

fn codegen_panic_inner<'tcx>(
fx: &mut FunctionCx<'_, '_, 'tcx>,
lang_item: rustc_hir::LangItem,
args: &[Value],
span: Span,
generic: Option<GenericArg<'tcx>>,
) {
let def_id = fx.tcx.require_lang_item(lang_item, Some(span));

let instance = Instance::mono(fx.tcx, def_id).polymorphize(fx.tcx);
let instance = if let Some(arg) = generic {
Instance::new(def_id, fx.tcx.mk_args(&[arg]))
} else {
Instance::mono(fx.tcx, def_id)
}
.polymorphize(fx.tcx);
let symbol_name = fx.tcx.symbol_name(instance).name;

fx.lib_call(
Expand Down
9 changes: 7 additions & 2 deletions compiler/rustc_codegen_ssa/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use rustc_hir::LangItem;
use rustc_middle::mir;
use rustc_middle::ty::{self, layout::TyAndLayout, Ty, TyCtxt};
use rustc_middle::ty::{self, layout::TyAndLayout, GenericArg, Ty, TyCtxt};
use rustc_span::Span;

use crate::base;
Expand Down Expand Up @@ -120,10 +120,15 @@ pub fn build_langcall<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
bx: &Bx,
span: Option<Span>,
li: LangItem,
generic: Option<GenericArg<'tcx>>,
) -> (Bx::FnAbiOfResult, Bx::Value) {
let tcx = bx.tcx();
let def_id = tcx.require_lang_item(li, span);
let instance = ty::Instance::mono(tcx, def_id);
let instance = if let Some(arg) = generic {
ty::Instance::new(def_id, tcx.mk_args(&[arg]))
} else {
ty::Instance::mono(tcx, def_id)
};
(bx.fn_abi_of_instance(instance, ty::List::empty()), bx.get_fn_addr(instance))
}

Expand Down
40 changes: 28 additions & 12 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mergeable_succ: bool,
) -> MergingSucc {
let span = terminator.source_info.span;
let cond = self.codegen_operand(bx, cond).immediate();
let mut cond = self.codegen_operand(bx, cond).immediate();
let mut const_cond = bx.const_to_opt_u128(cond, false).map(|c| c == 1);

// This case can currently arise only from functions marked
Expand All @@ -588,8 +588,10 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
return helper.funclet_br(self, bx, target, mergeable_succ);
}

// Pass the condition through llvm.expect for branch hinting.
let cond = bx.expect(cond, expected);
if bx.tcx().sess.opts.optimize != OptLevel::No {
// Pass the condition through llvm.expect for branch hinting.
cond = bx.expect(cond, expected);
}

// Create the failure block and the conditional branch to it.
let lltarget = helper.llbb_with_cleanup(self, target);
Expand All @@ -608,30 +610,40 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let location = self.get_caller_location(bx, terminator.source_info).immediate();

// Put together the arguments to the panic entry point.
let (lang_item, args) = match msg {
let (lang_item, args, generic) = match msg {
AssertKind::BoundsCheck { ref len, ref index } => {
let len = self.codegen_operand(bx, len).immediate();
let index = self.codegen_operand(bx, index).immediate();
// It's `fn panic_bounds_check(index: usize, len: usize)`,
// and `#[track_caller]` adds an implicit third argument.
(LangItem::PanicBoundsCheck, vec![index, len, location])
(LangItem::PanicBoundsCheck, vec![index, len, location], None)
}
AssertKind::MisalignedPointerDereference { ref required, ref found } => {
let required = self.codegen_operand(bx, required).immediate();
let found = self.codegen_operand(bx, found).immediate();
// It's `fn panic_misaligned_pointer_dereference(required: usize, found: usize)`,
// and `#[track_caller]` adds an implicit third argument.
(LangItem::PanicMisalignedPointerDereference, vec![required, found, location])
(LangItem::PanicMisalignedPointerDereference, vec![required, found, location], None)
}
AssertKind::OccupiedNiche { ref found, ref start, ref end } => {
let found = self.codegen_operand(bx, found);
let generic_arg = ty::GenericArg::from(found.layout.ty);
let found = found.immediate();
let start = self.codegen_operand(bx, start).immediate();
let end = self.codegen_operand(bx, end).immediate();
// It's `fn panic_occupied_niche<T>(found: T, start: T, end: T)`,
// and `#[track_caller]` adds an implicit fourth argument.
(LangItem::PanicOccupiedNiche, vec![found, start, end, location], Some(generic_arg))
}
_ => {
let msg = bx.const_str(msg.description());
// It's `pub fn panic(expr: &str)`, with the wide reference being passed
// as two arguments, and `#[track_caller]` adds an implicit third argument.
(LangItem::Panic, vec![msg.0, msg.1, location])
(LangItem::Panic, vec![msg.0, msg.1, location], None)
}
};

let (fn_abi, llfn) = common::build_langcall(bx, Some(span), lang_item);
let (fn_abi, llfn) = common::build_langcall(bx, Some(span), lang_item, generic);

// Codegen the actual panic invoke/call.
let merging_succ = helper.do_call(self, bx, fn_abi, llfn, &args, None, unwind, &[], false);
Expand All @@ -650,7 +662,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
self.set_debug_loc(bx, terminator.source_info);

// Obtain the panic entry point.
let (fn_abi, llfn) = common::build_langcall(bx, Some(span), reason.lang_item());
let (fn_abi, llfn) = common::build_langcall(bx, Some(span), reason.lang_item(), None);

// Codegen the actual panic invoke/call.
let merging_succ = helper.do_call(
Expand Down Expand Up @@ -711,8 +723,12 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let msg = bx.const_str(&msg_str);

// Obtain the panic entry point.
let (fn_abi, llfn) =
common::build_langcall(bx, Some(source_info.span), LangItem::PanicNounwind);
let (fn_abi, llfn) = common::build_langcall(
bx,
Some(source_info.span),
LangItem::PanicNounwind,
None,
);

// Codegen the actual panic invoke/call.
helper.do_call(
Expand Down Expand Up @@ -1586,7 +1602,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

self.set_debug_loc(&mut bx, mir::SourceInfo::outermost(self.mir.span));

let (fn_abi, fn_ptr) = common::build_langcall(&bx, None, reason.lang_item());
let (fn_abi, fn_ptr) = common::build_langcall(&bx, None, reason.lang_item(), None);
let fn_ty = bx.fn_decl_backend_type(&fn_abi);

let llret = bx.call(fn_ty, None, Some(&fn_abi), fn_ptr, &[], funclet.as_ref());
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,11 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
found: eval_to_int(found)?,
}
}
OccupiedNiche { ref found, ref start, ref end } => OccupiedNiche {
found: eval_to_int(found)?,
start: eval_to_int(start)?,
end: eval_to_int(end)?,
},
};
Err(ConstEvalErrKind::AssertFailure(err).into())
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ language_item_table! {
ConstPanicFmt, sym::const_panic_fmt, const_panic_fmt, Target::Fn, GenericRequirement::None;
PanicBoundsCheck, sym::panic_bounds_check, panic_bounds_check_fn, Target::Fn, GenericRequirement::Exact(0);
PanicMisalignedPointerDereference, sym::panic_misaligned_pointer_dereference, panic_misaligned_pointer_dereference_fn, Target::Fn, GenericRequirement::Exact(0);
PanicOccupiedNiche, sym::panic_occupied_niche, panic_occupied_niche_fn, Target::Fn, GenericRequirement::Exact(1);
PanicInfo, sym::panic_info, panic_info, Target::Struct, GenericRequirement::None;
PanicLocation, sym::panic_location, panic_location, Target::Struct, GenericRequirement::None;
PanicImpl, sym::panic_impl, panic_impl, Target::Fn, GenericRequirement::None;
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_middle/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ middle_assert_gen_resume_after_panic = `gen` fn or block cannot be further itera
middle_assert_misaligned_ptr_deref =
misaligned pointer dereference: address must be a multiple of {$required} but is {$found}
middle_assert_occupied_niche =
occupied niche: {$found} must be in {$start}..={$end}
middle_assert_op_overflow =
attempt to compute `{$left} {$op} {$right}`, which would overflow
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,7 @@ pub enum AssertKind<O> {
ResumedAfterReturn(CoroutineKind),
ResumedAfterPanic(CoroutineKind),
MisalignedPointerDereference { required: O, found: O },
OccupiedNiche { found: O, start: O, end: O },
}

#[derive(Clone, Debug, PartialEq, TyEncodable, TyDecodable, Hash, HashStable)]
Expand Down
16 changes: 14 additions & 2 deletions compiler/rustc_middle/src/mir/terminator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ impl<O> AssertKind<O> {
"`gen fn` should just keep returning `None` after panicking"
}

BoundsCheck { .. } | MisalignedPointerDereference { .. } => {
BoundsCheck { .. } | MisalignedPointerDereference { .. } | OccupiedNiche { .. } => {
bug!("Unexpected AssertKind")
}
}
Expand Down Expand Up @@ -220,6 +220,13 @@ impl<O> AssertKind<O> {
"\"misaligned pointer dereference: address must be a multiple of {{}} but is {{}}\", {required:?}, {found:?}"
)
}
OccupiedNiche { found, start, end } => {
write!(
f,
"\"occupied niche: {{}} must be in {{}}..={{}}\", {:?}, {:?}, {:?}",
found, start, end
)
}
_ => write!(f, "\"{}\"", self.description()),
}
}
Expand Down Expand Up @@ -254,8 +261,8 @@ impl<O> AssertKind<O> {
ResumedAfterPanic(CoroutineKind::Coroutine) => {
middle_assert_coroutine_resume_after_panic
}

MisalignedPointerDereference { .. } => middle_assert_misaligned_ptr_deref,
OccupiedNiche { .. } => middle_assert_occupied_niche,
}
}

Expand Down Expand Up @@ -292,6 +299,11 @@ impl<O> AssertKind<O> {
add!("required", format!("{required:#?}"));
add!("found", format!("{found:#?}"));
}
OccupiedNiche { found, start, end } => {
add!("found", format!("{found:?}"));
add!("start", format!("{start:?}"));
add!("end", format!("{end:?}"));
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,11 @@ macro_rules! make_mir_visitor {
self.visit_operand(required, location);
self.visit_operand(found, location);
}
OccupiedNiche { found, start, end } => {
self.visit_operand(found, location);
self.visit_operand(start, location);
self.visit_operand(end, location);
}
}
}

Expand Down
Loading

0 comments on commit eaa4669

Please sign in to comment.