Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Reduce size of InterpErrorInfo to 8 bytes #82116

Merged
merged 2 commits into from
Feb 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,29 +40,45 @@ pub fn struct_error<'tcx>(tcx: TyCtxtAt<'tcx>, msg: &str) -> DiagnosticBuilder<'
struct_span_err!(tcx.sess, tcx.span, E0080, "{}", msg)
}

#[cfg(target_arch = "x86_64")]
static_assert_size!(InterpErrorInfo<'_>, 8);

/// Packages the kind of error we got from the const code interpreter
/// up with a Rust-level backtrace of where the error occurred.
/// Thsese should always be constructed by calling `.into()` on
/// a `InterpError`. In `librustc_mir::interpret`, we have `throw_err_*`
/// macros for this.
#[derive(Debug)]
pub struct InterpErrorInfo<'tcx> {
pub kind: InterpError<'tcx>,
pub struct InterpErrorInfo<'tcx>(Box<InterpErrorInfoInner<'tcx>>);

#[derive(Debug)]
struct InterpErrorInfoInner<'tcx> {
kind: InterpError<'tcx>,
backtrace: Option<Box<Backtrace>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is already in a Box do we need a second Box here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this will be None in most cases (unless debugging the const evaluator), I'd leave this box in in order to not increase the regular size of the InterpErrorInfoInner.

Copy link
Contributor

@pickfire pickfire Feb 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if kind is used won't backtrace be used as well? Do we need a second indirection?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backtrace is only used if RUSTC_CTFE_BACKTRACE is set. In all other cases there will be no Box allocated. I'm not sure how large a Backtracestruct is, but if it's larger than8bytes, for all normal cases we don't waste space, as noBoxis allocated. It's fine to have arbitrary costs ifRUSTC_CTFE_BACKTRACE` is used, as that is already expensive by itself

}

impl fmt::Display for InterpErrorInfo<'_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.kind)
write!(f, "{}", self.0.kind)
}
}

impl InterpErrorInfo<'_> {
impl InterpErrorInfo<'tcx> {
pub fn print_backtrace(&self) {
if let Some(backtrace) = self.backtrace.as_ref() {
if let Some(backtrace) = self.0.backtrace.as_ref() {
print_backtrace(backtrace);
}
}

pub fn into_kind(self) -> InterpError<'tcx> {
let InterpErrorInfo(box InterpErrorInfoInner { kind, .. }) = self;
kind
}

#[inline]
pub fn kind(&self) -> &InterpError<'tcx> {
&self.0.kind
}
}

fn print_backtrace(backtrace: &Backtrace) {
Expand Down Expand Up @@ -108,7 +124,7 @@ impl<'tcx> From<InterpError<'tcx>> for InterpErrorInfo<'tcx> {
}
};

InterpErrorInfo { kind, backtrace }
InterpErrorInfo(Box::new(InterpErrorInfoInner { kind, backtrace }))
}
}

Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_mir/src/const_eval/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@ impl<'tcx> ConstEvalErr<'tcx> {
{
error.print_backtrace();
let stacktrace = ecx.generate_stacktrace();
ConstEvalErr { error: error.kind, stacktrace, span: span.unwrap_or_else(|| ecx.cur_span()) }
ConstEvalErr {
error: error.into_kind(),
stacktrace,
span: span.unwrap_or_else(|| ecx.cur_span()),
}
}

pub fn struct_error(
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ pub fn eval_to_const_value_raw_provider<'tcx>(
};
return eval_nullary_intrinsic(tcx, key.param_env, def_id, substs).map_err(|error| {
let span = tcx.def_span(def_id);
let error = ConstEvalErr { error: error.kind, stacktrace: vec![], span };
let error = ConstEvalErr { error: error.into_kind(), stacktrace: vec![], span };
error.report_as_error(tcx.at(span), "could not evaluate nullary intrinsic")
});
}
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_mir/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,8 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
Ok(Some(match ecx.load_mir(instance.def, None) {
Ok(body) => body,
Err(err) => {
if let err_unsup!(NoMirFor(did)) = err.kind {
let path = ecx.tcx.def_path_str(did);
if let err_unsup!(NoMirFor(did)) = err.kind() {
let path = ecx.tcx.def_path_str(*did);
return Err(ConstEvalErrKind::NeedsRfc(format!(
"calling extern function `{}`",
path
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ where
// an allocation, which we should avoid. When that happens,
// dedicated error variants should be introduced instead.
assert!(
!error.kind.allocates(),
!error.kind().allocates(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this stop making any sense now?
The point of this allocates thing was to make sure we don't allocate an error and then "catch" it (for perf reasons we wanted to avoid those allocations). But with this change we now allocate for all errors, so (a) it seems silly to also separately Box some error kinds again, and (b) this check here makes no sense since we always allocate. So shouldn't we get rid of the extra nested Box and the allocates stuff?

(Btw I'd appreciate a Cc for changes like this that affect the Miri/CTFE engine.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unboxing of UndefinedBehaviorInfo::InvalidUninitBytes content would be one thing to consider, but generally this check makes as much sense now as it made before. Heap allocation inside & outside cannot be easily merged together.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check makes as much sense now as it made before

If that means it didn't make any sense before, that's okay and we should still remove it. ;)

But I feel like the difference between 0 and 1 allocation is much bigger than between 1 and 2, so I am not sure this check is worth keeping. OTOH, maybe the expensive bit we should protect against isn't the allocation itself but the string formatting in the format!-using errors. In that case we should probably rename allocates to pre_formatted or so (and unbox InvalidUninitBytes as you said).

"interning encountered allocating error: {}",
error
);
Expand Down
27 changes: 15 additions & 12 deletions compiler/rustc_mir/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use std::ops::RangeInclusive;

use rustc_data_structures::fx::FxHashSet;
use rustc_hir as hir;
use rustc_middle::mir::interpret::{InterpError, InterpErrorInfo};
use rustc_middle::mir::interpret::InterpError;
use rustc_middle::ty;
use rustc_middle::ty::layout::TyAndLayout;
use rustc_span::symbol::{sym, Symbol};
Expand Down Expand Up @@ -83,14 +83,17 @@ macro_rules! try_validation {
Ok(x) => x,
// We catch the error and turn it into a validation failure. We are okay with
// allocation here as this can only slow down builds that fail anyway.
$( $( Err(InterpErrorInfo { kind: $p, .. }) )|+ =>
throw_validation_failure!(
$where,
{ $( $what_fmt ),+ } $( expected { $( $expected_fmt ),+ } )?
),
)+
#[allow(unreachable_patterns)]
Err(e) => Err::<!, _>(e)?,
Err(e) => match e.kind() {
$(
$($p)|+ =>
throw_validation_failure!(
$where,
{ $( $what_fmt ),+ } $( expected { $( $expected_fmt ),+ } )?
)
),+,
#[allow(unreachable_patterns)]
_ => Err::<!, _>(e)?,
}
}
}};
}
Expand Down Expand Up @@ -877,7 +880,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
Err(err) => {
// For some errors we might be able to provide extra information.
// (This custom logic does not fit the `try_validation!` macro.)
match err.kind {
match err.kind() {
err_ub!(InvalidUninitBytes(Some(access))) => {
// Some byte was uninitialized, determine which
// element that byte belongs to so we can
Expand Down Expand Up @@ -935,10 +938,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
match visitor.visit_value(op) {
Ok(()) => Ok(()),
// Pass through validation failures.
Err(err) if matches!(err.kind, err_ub!(ValidationFailure { .. })) => Err(err),
Err(err) if matches!(err.kind(), err_ub!(ValidationFailure { .. })) => Err(err),
// Also pass through InvalidProgram, those just indicate that we could not
// validate and each caller will know best what to do with them.
Err(err) if matches!(err.kind, InterpError::InvalidProgram(_)) => Err(err),
Err(err) if matches!(err.kind(), InterpError::InvalidProgram(_)) => Err(err),
// Avoid other errors as those do not show *where* in the value the issue lies.
Err(err) => {
err.print_backtrace();
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir/src/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
// an allocation, which we should avoid. When that happens,
// dedicated error variants should be introduced instead.
assert!(
!error.kind.allocates(),
!error.kind().allocates(),
"const-prop encountered allocating error: {}",
error
);
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-23458.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![feature(llvm_asm)]

// compile-flags: -Ccodegen-units=1
// build-fail
// only-x86_64

Expand Down