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

Make panicking in constants work with the 2021 edition #86830

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
4 changes: 4 additions & 0 deletions compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,10 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
&args.iter().map(|arg| arg.immediate()).collect::<Vec<_>>(),
None,
),
sym::panic_ctfe_hook => {
// Does nothing at runtime.
return;
}
sym::likely => {
let expect = self.get_intrinsic(&("llvm.expect.i1"));
self.call(expect, &[args[0].immediate(), self.const_bool(true)], None)
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 @@ -327,6 +327,7 @@ language_item_table! {

FromFrom, sym::from, from_fn, Target::Method(MethodKind::Trait { body: false });

Option, sym::Option, option_ty, Target::Enum;
OptionSome, sym::Some, option_some_variant, Target::Variant;
OptionNone, sym::None, option_none_variant, Target::Variant;

Expand Down
9 changes: 6 additions & 3 deletions compiler/rustc_mir/src/const_eval/fn_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@ fn is_const_fn_raw(tcx: TyCtxt<'_>, def_id: DefId) -> bool {
if let hir::Node::ForeignItem(hir::ForeignItem { kind: hir::ForeignItemKind::Fn(..), .. }) =
node
{
// Intrinsics use `rustc_const_{un,}stable` attributes to indicate constness. All other
// foreign items cannot be evaluated at compile-time.
if let Abi::RustIntrinsic | Abi::PlatformIntrinsic = tcx.hir().get_foreign_abi(hir_id) {
// Intrinsics use `rustc_const_{un,}stable` attributes to indicate constness.
// `panic_impl` also does in order to pass const checks (it is never evaluated at compile
// time).
if let Abi::Rust | Abi::RustIntrinsic | Abi::PlatformIntrinsic =
Copy link
Member

Choose a reason for hiding this comment

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

Abi::Rust is not an intrinsic ABI, this looks wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I assume this is for the panic_impl?

This is a rather bad hack, I'd strongly prefer it we can avoid it. I have no idea if it has adverse side-effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to special-case the panic_impl lang item only, but it is None in libcore, despite the #[lang = "panic_impl"] on panic_impl.

Copy link
Member

Choose a reason for hiding this comment

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

Hm... but now that you are allowing a stable ABI here (as in, an ABI that can be used by stable code), the ABI check just does not make any sense any more this way; we might as well remove it entirely and always call lookup_const_stability.

And then we need to somehow be sure that this cannot be exploited by user code, and that is the part I am unsure about.

tcx.hir().get_foreign_abi(hir_id)
{
tcx.lookup_const_stability(def_id).is_some()
} else {
false
Expand Down
29 changes: 29 additions & 0 deletions compiler/rustc_mir/src/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,35 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter<'mir,
);
ecx.write_scalar(Scalar::Ptr(ptr), dest)?;
}
sym::panic_ctfe_hook => {
// Option<&str>
assert!(args.len() == 1);

debug!("panic_ctfe_hook: invoked with {:?}", args[0]);

let (_, idx) = ecx.read_discriminant(&args[0])?;
let some = match idx.index() {
0 => {
// None
// For now, this is unreachable code - you cannot construct a non-literal
Copy link
Member

@RalfJung RalfJung Jul 3, 2021

Choose a reason for hiding this comment

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

Why is this unreachable? What happens when I do panic!("{}", "hello") in a const fn in the new edition?

// `fmt::Arguments` without `impl const Display`, which we don't currently
// provide.
Copy link
Member

Choose a reason for hiding this comment

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

I can't parse this sentence. What does the last "which" refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To the impl const Display

Copy link
Member

Choose a reason for hiding this comment

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

So maybe ", and we currently do not provide such an impl"?

But I also don't see what this has to do with impl const Display...

Copy link
Member

Choose a reason for hiding this comment

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

Actually I think my main confusion is around the term "non-literal fmt::Arguments". You also use it in the panic_ctfe_hook doc comment, but I do not know what you mean by that term.

unreachable!("non-literal `fmt::Arguments` used in const panic");
}
1 => {
// Some
ecx.operand_downcast(&args[0], idx)?
}
_ => unreachable!("encountered `Option` with variant {:?}", idx),
};

let ref_msg = ecx.operand_field(&some, 0)?;
let msg_place = ecx.deref_operand(&ref_msg)?;
let msg = Symbol::intern(ecx.read_str(&msg_place)?);
let span = ecx.find_closest_untracked_caller_location();
let (file, line, col) = ecx.location_triple_for_span(span);
return Err(ConstEvalErrKind::Panic { msg, file, line, col }.into());
}
_ => {
return Err(ConstEvalErrKind::NeedsRfc(format!(
"calling intrinsic `{}`",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Stop inside the most nested non-`#[track_caller]` function,
// before ever reaching its caller (which is irrelevant).
if !callee.def.requires_caller_location(*self.tcx) {
debug!(
"find_closest_untracked_caller_location: result (inlined) is {:?}",
source_info.span
);
return source_info.span;
}
source_info.span = callsite_span;
Expand All @@ -66,6 +70,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Stop inside the most nested non-`#[track_caller]` function,
// before ever reaching its caller (which is irrelevant).
if !frame.instance.def.requires_caller_location(*self.tcx) {
debug!("find_closest_untracked_caller_location: result is {:?}", source_info.span);
return source_info.span;
}
}
Expand Down
8 changes: 6 additions & 2 deletions compiler/rustc_mir/src/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,12 @@ where
&self,
val: &ImmTy<'tcx, M::PointerTag>,
) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
let pointee_type =
val.layout.ty.builtin_deref(true).expect("`ref_to_mplace` called on non-ptr type").ty;
let pointee_type = val
.layout
.ty
.builtin_deref(true)
.unwrap_or_else(|| panic!("`ref_to_mplace` called on non-ptr type {}", val.layout.ty))
.ty;
let layout = self.layout_of(pointee_type)?;
let (ptr, meta) = match **val {
Immediate::Scalar(ptr) => (ptr.check_init()?, MemPlaceMeta::None),
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ symbols! {
any,
arbitrary_enum_discriminant,
arbitrary_self_types,
arguments_as_str,
arith_offset,
arm,
arm_target_feature,
Expand Down Expand Up @@ -853,6 +854,8 @@ symbols! {
panic_2021,
panic_abort,
panic_bounds_check,
panic_ctfe_hook,
panic_fmt,
panic_handler,
panic_impl,
panic_implementation,
Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_typeck/src/check/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::errors::{
};
use crate::require_same_types;

use hir::LangItem;
use rustc_errors::{pluralize, struct_span_err};
use rustc_hir as hir;
use rustc_middle::traits::{ObligationCause, ObligationCauseCode};
Expand Down Expand Up @@ -297,6 +298,18 @@ pub fn check_intrinsic_type(tcx: TyCtxt<'_>, it: &hir::ForeignItem<'_>) {
(0, vec![tcx.types.usize, tcx.types.usize], tcx.mk_mut_ptr(tcx.types.u8))
}

sym::panic_ctfe_hook => {
let br = ty::BoundRegion { var: ty::BoundVar::from_u32(0), kind: ty::BrAnon(0) };
let ref_str = tcx
.mk_imm_ref(tcx.mk_region(ty::ReLateBound(ty::INNERMOST, br)), tcx.types.str_);
let option_def_id = tcx.require_lang_item(LangItem::Option, None);
let adt_def = tcx.adt_def(option_def_id);
let substs = tcx.mk_substs([ref_str.into()].iter());
let opt = tcx.mk_adt(&adt_def, substs);

(0, vec![opt], tcx.types.unit)
}

sym::ptr_offset_from => {
(1, vec![tcx.mk_imm_ptr(param(0)), tcx.mk_imm_ptr(param(0))], tcx.types.isize)
}
Expand Down
4 changes: 2 additions & 2 deletions library/core/src/fmt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ impl<'a> Arguments<'a> {
#[doc(hidden)]
#[inline]
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
pub fn new_v1(pieces: &'a [&'static str], args: &'a [ArgumentV1<'a>]) -> Arguments<'a> {
pub const fn new_v1(pieces: &'a [&'static str], args: &'a [ArgumentV1<'a>]) -> Arguments<'a> {
Arguments { pieces, fmt: None, args }
}

Expand All @@ -347,7 +347,7 @@ impl<'a> Arguments<'a> {
#[doc(hidden)]
#[inline]
#[unstable(feature = "fmt_internals", reason = "internal to format_args!", issue = "none")]
pub fn new_v1_formatted(
pub const fn new_v1_formatted(
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
pieces: &'a [&'static str],
args: &'a [ArgumentV1<'a>],
fmt: &'a [rt::v1::Argument],
Expand Down
6 changes: 6 additions & 0 deletions library/core/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,7 @@ extern "rust-intrinsic" {
///
/// A more user-friendly and stable version of this operation is
/// [`std::process::abort`](../../std/process/fn.abort.html).
#[rustc_const_unstable(feature = "const_abort", issue = "none")]
Copy link
Member

Choose a reason for hiding this comment

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

Please explain that this is for the const panic machinery, and not actually implemented (I assume actually reaching this during CTFE will ICE)

pub fn abort() -> !;

/// Informs the optimizer that this point in the code is not reachable,
Expand Down Expand Up @@ -1907,6 +1908,11 @@ extern "rust-intrinsic" {
/// Allocate at compile time. Should not be called at runtime.
#[rustc_const_unstable(feature = "const_heap", issue = "79597")]
pub fn const_allocate(size: usize, align: usize) -> *mut u8;

/// Implementation detail of the `const_panic` feature.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
#[rustc_const_unstable(feature = "panic_ctfe_hook", issue = "none")]
#[cfg(not(bootstrap))]
pub fn panic_ctfe_hook(message: Option<&str>);
}

// Some functions are defined here because they accidentally got made
Expand Down
4 changes: 4 additions & 0 deletions library/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@
#![feature(asm)]
#![feature(bool_to_option)]
#![feature(cfg_target_has_atomic)]
#![feature(const_abort)]
#![feature(const_arguments_as_str)]
#![feature(const_heap)]
#![feature(const_alloc_layout)]
#![feature(const_assert_type)]
Expand All @@ -81,6 +83,7 @@
#![feature(const_mut_refs)]
#![feature(const_refs_to_cell)]
#![feature(const_panic)]
#![cfg_attr(not(bootstrap), feature(const_panic_impl))]
#![feature(const_pin)]
#![feature(const_fn_union)]
#![feature(const_impl_trait)]
Expand Down Expand Up @@ -124,6 +127,7 @@
#![feature(exhaustive_patterns)]
#![feature(no_core)]
#![feature(auto_traits)]
#![cfg_attr(not(bootstrap), feature(panic_ctfe_hook))]
#![feature(prelude_import)]
#![feature(ptr_metadata)]
#![feature(repr_simd, platform_intrinsics)]
Expand Down
1 change: 1 addition & 0 deletions library/core/src/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ use crate::{
#[derive(Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
#[rustc_diagnostic_item = "option_type"]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(bootstrap), lang = "Option")]
pub enum Option<T> {
/// No value
#[lang = "None"]
Expand Down
14 changes: 7 additions & 7 deletions library/core/src/panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub macro panic_2021 {
#[stable(feature = "panic_hooks", since = "1.10.0")]
#[derive(Debug)]
pub struct PanicInfo<'a> {
payload: &'a (dyn Any + Send),
payload: Option<&'a (dyn Any + Send)>,
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
}
Expand All @@ -78,12 +78,11 @@ impl<'a> PanicInfo<'a> {
)]
#[doc(hidden)]
#[inline]
pub fn internal_constructor(
pub const fn internal_constructor(
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
) -> Self {
struct NoPayload;
PanicInfo { location, message, payload: &NoPayload }
PanicInfo { location, message, payload: None }
}

#[unstable(
Expand All @@ -94,7 +93,7 @@ impl<'a> PanicInfo<'a> {
#[doc(hidden)]
#[inline]
pub fn set_payload(&mut self, info: &'a (dyn Any + Send)) {
self.payload = info;
self.payload = Some(info);
}

/// Returns the payload associated with the panic.
Expand All @@ -120,7 +119,8 @@ impl<'a> PanicInfo<'a> {
/// ```
#[stable(feature = "panic_hooks", since = "1.10.0")]
pub fn payload(&self) -> &(dyn Any + Send) {
self.payload
struct NoPayload;
self.payload.unwrap_or(&NoPayload)
}

/// If the `panic!` macro from the `core` crate (not from `std`)
Expand Down Expand Up @@ -169,7 +169,7 @@ impl fmt::Display for PanicInfo<'_> {
formatter.write_str("panicked at ")?;
if let Some(message) = self.message {
write!(formatter, "'{}', ", message)?
} else if let Some(payload) = self.payload.downcast_ref::<&'static str>() {
} else if let Some(payload) = self.payload().downcast_ref::<&'static str>() {
write!(formatter, "'{}', ", payload)?
}
// NOTE: we cannot use downcast_ref::<String>() here
Expand Down
34 changes: 34 additions & 0 deletions library/core/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ fn panic_bounds_check(index: usize, len: usize) -> ! {
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[track_caller]
#[cfg(bootstrap)]
pub fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
if cfg!(feature = "panic_immediate_abort") {
super::intrinsics::abort()
Expand All @@ -92,6 +93,39 @@ pub fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
unsafe { panic_impl(&pi) }
}

/// The underlying implementation of libcore's `panic!` macro when formatting is used.
#[cold]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[cfg_attr(feature = "panic_immediate_abort", inline)]
#[track_caller]
#[cfg(not(bootstrap))]
pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
if cfg!(feature = "panic_immediate_abort") {
super::intrinsics::abort()
}

// NOTE This function never crosses the FFI boundary; it's a Rust-to-Rust call
// that gets resolved to the `#[panic_handler]` function.
extern "Rust" {
#[lang = "panic_impl"]
#[rustc_const_unstable(feature = "const_panic_impl", issue = "none")]
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved
fn panic_impl(pi: &PanicInfo<'_>) -> !;
}

#[cfg(not(bootstrap))]
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved
// SAFETY: the intrinsic is always safe to call.
unsafe {
// If we're const-evaluating this panic, this call will abort evaluation and unwind.
// The code below is only reachable during runtime.
core::intrinsics::panic_ctfe_hook(fmt.as_str());
jonas-schievink marked this conversation as resolved.
Show resolved Hide resolved
}

let pi = PanicInfo::internal_constructor(Some(&fmt), Location::caller());

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
}

#[derive(Debug)]
#[doc(hidden)]
pub enum AssertKind {
Expand Down
1 change: 0 additions & 1 deletion src/test/ui/borrowck/issue-64453.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ struct Value;

static settings_dir: String = format!("");
//~^ ERROR calls in statics are limited to constant functions
//~| ERROR calls in statics are limited to constant functions
Copy link
Member

@RalfJung RalfJung Jul 3, 2021

Choose a reason for hiding this comment

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

So there's one error less here... which function is the remaining error from?
Can you add a format_args!, just to make sure that that is still rejected inside const? Probably something like

const CALL_FMT_ARGS: () = {
  let _ = format_args!("");
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The remaining error is from the conversion of fmt::Arguments to String.

format_args!("") does in fact compile now, since panic!("") expands to that. Is there some way to make it only const when used through panic!("")?

Copy link
Member

Choose a reason for hiding this comment

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

format_args!("") does in fact compile now

I feared this might happen; we need to fix that (so please definitely add a test).

Is there some way to make it only const when used through panic!("")?

const checking happens after macro expansion, so this will be a bit tricky, but I still think adding a special hack in const checking is the best way here. @oli-obk would have a better idea for how to best to that; I hope they are back soon. :D


fn from_string(_: String) -> Value {
Value
Expand Down
12 changes: 2 additions & 10 deletions src/test/ui/borrowck/issue-64453.stderr
Original file line number Diff line number Diff line change
@@ -1,17 +1,9 @@
error[E0507]: cannot move out of static item `settings_dir`
--> $DIR/issue-64453.rs:14:37
--> $DIR/issue-64453.rs:13:37
|
LL | let settings_data = from_string(settings_dir);
| ^^^^^^^^^^^^ move occurs because `settings_dir` has type `String`, which does not implement the `Copy` trait

error[E0015]: calls in statics are limited to constant functions, tuple structs and tuple variants
--> $DIR/issue-64453.rs:4:31
|
LL | static settings_dir: String = format!("");
| ^^^^^^^^^^^
|
= note: this error originates in the macro `$crate::__export::format_args` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0015]: calls in statics are limited to constant functions, tuple structs and tuple variants
--> $DIR/issue-64453.rs:4:31
|
Expand All @@ -20,7 +12,7 @@ LL | static settings_dir: String = format!("");
|
= note: this error originates in the macro `format` (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 3 previous errors
error: aborting due to 2 previous errors

Some errors have detailed explanations: E0015, E0507.
For more information about an error, try `rustc --explain E0015`.
31 changes: 31 additions & 0 deletions src/test/ui/consts/const-eval/const_panic_2021.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// edition:2021

// NB: panic macros without arguments share the 2015/2018 edition hook
// We cannot annotate the expected error in the test because it point at libcore
// FIXME: this is a very bad error message

#![feature(const_panic)]
#![allow(non_fmt_panic)]
#![crate_type = "lib"]

const Z: () = std::panic!("cheese");

const Z2: () = std::panic!();
//~^ ERROR evaluation of constant value failed

const Y: () = std::unreachable!();
//~^ ERROR evaluation of constant value failed

const X: () = std::unimplemented!();
//~^ ERROR evaluation of constant value failed

const Z_CORE: () = core::panic!("cheese");

const Z2_CORE: () = core::panic!();
//~^ ERROR evaluation of constant value failed

const Y_CORE: () = core::unreachable!();
//~^ ERROR evaluation of constant value failed

const X_CORE: () = core::unimplemented!();
//~^ ERROR evaluation of constant value failed
Loading