Skip to content

Commit

Permalink
Auto merge of rust-lang#121767 - saethlin:ubcheck-intrinsic, r=<try>
Browse files Browse the repository at this point in the history
[nightmarish hacks within] Lower UB checks as a new kind of intrinsic, not calls

I _think_ one reason the UB checks have the compile time overhead that they do is that they impede MIR optimizations by being calls, which are penalized heavily in MIR inlining, and also simply complicate the MIR we generate.

So this is my (ongoing) effort to fix that. Currently the code is strewn with terrible hacks that I'll figure out how to do better later. Right now I just want to establish if this is a path to reducing their compile-time overhead.

r? `@ghost`
  • Loading branch information
bors committed Feb 28, 2024
2 parents bf9c7a6 + 9ae476b commit 9f42d34
Show file tree
Hide file tree
Showing 66 changed files with 1,155 additions and 598 deletions.
4 changes: 4 additions & 0 deletions compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,10 @@ impl<'cx, 'tcx, R> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx, R> for MirBorro
NonDivergingIntrinsic::CopyNonOverlapping(..) => span_bug!(
span,
"Unexpected CopyNonOverlapping, should only appear after lower_intrinsics",
),
NonDivergingIntrinsic::UbCheck { .. } => span_bug!(
span,
"Unexpected UbCheck, should only appear after lower_intrinsics",
)
}
// Only relevant for mir typeck
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_borrowck/src/polonius/loan_invalidations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,18 @@ impl<'cx, 'tcx> Visitor<'tcx> for LoanInvalidationsGenerator<'cx, 'tcx> {
self.consume_operand(location, dst);
self.consume_operand(location, count);
}
StatementKind::Intrinsic(box NonDivergingIntrinsic::UbCheck {
kind: _,
func,
args,
destination,
source_info: _,
fn_span: _,
}) => {
self.consume_operand(location, func);
self.consume_operand(location, args);
self.mutate_place(location, *destination, Shallow(None));
}
// Only relevant for mir typeck
StatementKind::AscribeUserType(..)
// Only relevant for liveness and unsafeck
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1367,6 +1367,10 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
stmt.source_info.span,
"Unexpected NonDivergingIntrinsic::CopyNonOverlapping, should only appear after lowering_intrinsics",
),
NonDivergingIntrinsic::UbCheck { .. } => span_bug!(
stmt.source_info.span,
"Unexpected NonDivergingIntrinsic::UbCheck, should only appear after lowering_intrinsics",
),
},
StatementKind::FakeRead(..)
| StatementKind::StorageLive(..)
Expand Down Expand Up @@ -2001,7 +2005,6 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
ConstraintCategory::SizedBound,
);
}
&Rvalue::NullaryOp(NullOp::DebugAssertions, _) => {}

Rvalue::ShallowInitBox(operand, ty) => {
self.check_operand(operand, location);
Expand Down
12 changes: 3 additions & 9 deletions compiler/rustc_codegen_cranelift/src/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -767,15 +767,6 @@ fn codegen_stmt<'tcx>(
NullOp::OffsetOf(fields) => {
layout.offset_of_subfield(fx, fields.iter()).bytes()
}
NullOp::DebugAssertions => {
let val = fx.tcx.sess.opts.debug_assertions;
let val = CValue::by_val(
fx.bcx.ins().iconst(types::I8, i64::try_from(val).unwrap()),
fx.layout_of(fx.tcx.types.bool),
);
lval.write_cvalue(fx, val);
return;
}
};
let val = CValue::by_val(
fx.bcx.ins().iconst(fx.pointer_type, i64::try_from(val).unwrap()),
Expand Down Expand Up @@ -845,6 +836,9 @@ fn codegen_stmt<'tcx>(
};
fx.bcx.call_memcpy(fx.target_config, dst, src, bytes);
}
NonDivergingIntrinsic::UbCheck { .. } => {
todo!() // FIXME
}
},
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_cranelift/src/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,7 @@ pub(crate) fn mir_operand_get_const_val<'tcx>(
StatementKind::Intrinsic(ref intrinsic) => match **intrinsic {
NonDivergingIntrinsic::CopyNonOverlapping(..) => return None,
NonDivergingIntrinsic::Assume(..) => {}
NonDivergingIntrinsic::UbCheck { .. } => {}
},
// conservative handling
StatementKind::Assign(_)
Expand Down
42 changes: 26 additions & 16 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,19 @@ use std::cmp;
// can happen when BB jumps directly to its successor and the successor has no
// other predecessors.
#[derive(Debug, PartialEq)]
enum MergingSucc {
pub(crate) enum MergingSucc {
False,
True,
}

/// Used by `FunctionCx::codegen_terminator` for emitting common patterns
/// e.g., creating a basic block, calling a function, etc.
struct TerminatorCodegenHelper<'tcx> {
bb: mir::BasicBlock,
terminator: &'tcx mir::Terminator<'tcx>,
pub(crate) struct TerminatorCodegenHelper<'term, 'tcx> {
pub(crate) bb: mir::BasicBlock,
pub(crate) terminator: &'term mir::Terminator<'tcx>,
}

impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
impl<'term, 'a, 'tcx> TerminatorCodegenHelper<'term, 'tcx> {
/// Returns the appropriate `Funclet` for the current funclet, if on MSVC,
/// either already previously cached, or newly created, by `landing_pad_for`.
fn funclet<'b, Bx: BuilderMethods<'a, 'tcx>>(
Expand Down Expand Up @@ -98,6 +98,10 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
fx: &mut FunctionCx<'a, 'tcx, Bx>,
target: mir::BasicBlock,
) -> (bool, bool) {
if self.bb > fx.mir.basic_blocks.len().into() {
return (false, false);
}

if let Some(ref cleanup_kinds) = fx.cleanup_kinds {
let funclet_bb = cleanup_kinds[self.bb].funclet_bb(self.bb);
let target_funclet = cleanup_kinds[target].funclet_bb(target);
Expand Down Expand Up @@ -226,8 +230,10 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
MergingSucc::False
} else {
let llret = bx.call(fn_ty, fn_attrs, Some(fn_abi), fn_ptr, llargs, self.funclet(fx));
if fx.mir[self.bb].is_cleanup {
bx.apply_attrs_to_cleanup_callsite(llret);
if let Some(bb) = fx.mir.basic_blocks.get(self.bb) {
if bb.is_cleanup {
bx.apply_attrs_to_cleanup_callsite(llret);
}
}

if let Some((ret_dest, target)) = destination {
Expand Down Expand Up @@ -296,7 +302,11 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
/// Codegen implementations for some terminator variants.
impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
/// Generates code for a `Resume` terminator.
fn codegen_resume_terminator(&mut self, helper: TerminatorCodegenHelper<'tcx>, bx: &mut Bx) {
fn codegen_resume_terminator(
&mut self,
helper: TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
) {
if let Some(funclet) = helper.funclet(self) {
bx.cleanup_ret(funclet, None);
} else {
Expand All @@ -313,7 +323,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

fn codegen_switchint_terminator(
&mut self,
helper: TerminatorCodegenHelper<'tcx>,
helper: TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
discr: &mir::Operand<'tcx>,
targets: &SwitchTargets,
Expand Down Expand Up @@ -451,7 +461,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
#[tracing::instrument(level = "trace", skip(self, helper, bx))]
fn codegen_drop_terminator(
&mut self,
helper: TerminatorCodegenHelper<'tcx>,
helper: TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
location: mir::Place<'tcx>,
target: mir::BasicBlock,
Expand Down Expand Up @@ -569,7 +579,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

fn codegen_assert_terminator(
&mut self,
helper: TerminatorCodegenHelper<'tcx>,
helper: TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
terminator: &mir::Terminator<'tcx>,
cond: &mir::Operand<'tcx>,
Expand Down Expand Up @@ -649,7 +659,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

fn codegen_terminate_terminator(
&mut self,
helper: TerminatorCodegenHelper<'tcx>,
helper: TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
terminator: &mir::Terminator<'tcx>,
reason: UnwindTerminateReason,
Expand Down Expand Up @@ -678,7 +688,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
/// Returns `Some` if this is indeed a panic intrinsic and codegen is done.
fn codegen_panic_intrinsic(
&mut self,
helper: &TerminatorCodegenHelper<'tcx>,
helper: &TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
intrinsic: Option<Symbol>,
instance: Option<Instance<'tcx>>,
Expand Down Expand Up @@ -744,9 +754,9 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
}

fn codegen_call_terminator(
pub(crate) fn codegen_call_terminator(
&mut self,
helper: TerminatorCodegenHelper<'tcx>,
helper: TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
terminator: &mir::Terminator<'tcx>,
func: &mir::Operand<'tcx>,
Expand Down Expand Up @@ -1068,7 +1078,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {

fn codegen_asm_terminator(
&mut self,
helper: TerminatorCodegenHelper<'tcx>,
helper: TerminatorCodegenHelper<'_, 'tcx>,
bx: &mut Bx,
terminator: &mir::Terminator<'tcx>,
template: &[ast::InlineAsmTemplatePiece],
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -684,10 +684,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let val = layout.offset_of_subfield(bx.cx(), fields.iter()).bytes();
bx.cx().const_usize(val)
}
mir::NullOp::DebugAssertions => {
let val = bx.tcx().sess.opts.debug_assertions;
bx.cx().const_bool(val)
}
};
let tcx = self.cx.tcx();
OperandRef {
Expand Down
40 changes: 40 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/statement.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
use rustc_middle::mir;
use rustc_middle::mir::NonDivergingIntrinsic;
use rustc_session::config::OptLevel;
use rustc_span::source_map::respan;

use super::FunctionCx;
use super::LocalRef;
use crate::mir::block::TerminatorCodegenHelper;
use crate::traits::*;

impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
Expand Down Expand Up @@ -90,6 +92,44 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let src = src_val.immediate();
bx.memcpy(dst, align, src, align, bytes, crate::MemFlags::empty());
}
mir::StatementKind::Intrinsic(box NonDivergingIntrinsic::UbCheck {
kind: _,
ref func,
ref args,
destination,
fn_span,
source_info,
}) => {
if bx.tcx().sess.opts.debug_assertions {
let bb = mir::BasicBlock::MAX;
let args = vec![respan(source_info.span, args.clone())];
let terminator = mir::Terminator {
source_info,
kind: mir::TerminatorKind::Call {
func: func.clone(),
args: args.clone(),
destination,
target: Some(bb),
unwind: mir::UnwindAction::Unreachable,
call_source: mir::CallSource::Normal,
fn_span,
},
};
let helper = TerminatorCodegenHelper { bb, terminator: &terminator };
self.codegen_call_terminator(
helper,
bx,
&terminator,
func,
args.as_slice(),
destination,
Some(bb),
mir::UnwindAction::Unreachable,
fn_span,
true, // mergeable_succ
);
}
}
mir::StatementKind::FakeRead(..)
| mir::StatementKind::Retag { .. }
| mir::StatementKind::AscribeUserType(..)
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_const_eval/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ impl<'mir, 'tcx: 'mir> CompileTimeEvalContext<'mir, 'tcx> {
dest,
ret,
mir::UnwindAction::Unreachable,
false,
)?;
Ok(ControlFlow::Break(()))
} else {
Expand Down
12 changes: 12 additions & 0 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ pub enum StackPopCleanup {
/// wants them leaked to intern what they need (and just throw away
/// the entire `ecx` when it is done).
Root { cleanup: bool },

/// FIXME: We're returning from a UbCheck call, don't do anything.
Nothing,
}

/// State of a local variable including a memoized layout
Expand Down Expand Up @@ -928,6 +931,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let cleanup = match return_to_block {
StackPopCleanup::Goto { .. } => true,
StackPopCleanup::Root { cleanup, .. } => cleanup,
StackPopCleanup::Nothing => true,
};
if cleanup {
// We need to take the locals out, since we need to mutate while iterating.
Expand Down Expand Up @@ -964,6 +968,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
StackPopCleanup::Root { .. } => {
panic!("encountered StackPopCleanup::Root when unwinding!")
}
StackPopCleanup::Nothing => {
panic!("encountered StackPopCleanup::Nothing when unwinding!")
}
};
// This must be the very last thing that happens, since it can in fact push a new stack frame.
self.unwind_to_block(unwind)
Expand All @@ -978,6 +985,11 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
);
Ok(())
}
StackPopCleanup::Nothing => {
// Advance the program counter.
self.frame_mut().loc.as_mut().left().unwrap().statement_index += 1;
Ok(())
}
}
}
}
Expand Down

0 comments on commit 9f42d34

Please sign in to comment.