Skip to content

Commit

Permalink
Add new rustc_panic_abort_runtime attribute for libpanic_abort
Browse files Browse the repository at this point in the history
Supersedes #66311

This replaces the hack in `bootstrap`, allowing the special handling for
`libpanic_abort` to be encoded into the crate source itself, rather than
existing as special knowledge in the build system. This will allow Miri
(and any other users of Xargo) to correctly build `libpanic_abort`
without relying on `bootstrap` or custom wrapepr hacks.

The trickeist part of this PR is how we handle LLVM. The `emscripten`
target family requires the "-enable-emscripten-cxx-exceptions" flag
to be passed to LLVM when the panic strategy is set to "unwind".

Unfortunately, the location of this emscripten-specific check ends up
creating a circular dependency between LLVM and attribute resoltion.
When we check the panic strategy, we need to have already parsed crate
attributes, so that we determine if `rustc_panic_abort_runtime` was set.
However, attribute parsing requires LLVM to be initialized, so that we
can proerly handle cfg-gating of target-specific features. However, the
whole point of checking the panic strategy is to determinne which flags
to use during LLVM initialization!

To break this circular dependency, we explicitly set the
"-enable-emscripten-cxx-exceptions" in LLVM's argument-parsing framework
(using public but poorly-documented APIs). While this approach is
unfortunate, it only affects emscripten, and only modifies a
command-line flag which is never used until much later (when we create a
`PassManager`).
  • Loading branch information
Aaron1011 committed Dec 21, 2019
1 parent 0a440b1 commit 635c76b
Show file tree
Hide file tree
Showing 12 changed files with 84 additions and 12 deletions.
10 changes: 9 additions & 1 deletion src/bootstrap/bin/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,22 @@ fn main() {
// other crate intentionally as this is the only crate for now that we
// ship with panic=abort.
//
// This hack is being replaced with a new "rustc_panic_abort_runtime"
// attribute, which moves the special-casing out of `bootstrap` and
// into the compiler itself. Until this change makes its way into
// the bootstrap compiler, we still need this hack when building
// stage0. Once the boostrap compiler is updated, we can remove
// this check entirely.
// cfg(bootstrap): (Added to make this show up when searching for "cfg(bootstrap)")
//
// This... is a bit of a hack how we detect this. Ideally this
// information should be encoded in the crate I guess? Would likely
// require an RFC amendment to RFC 1513, however.
//
// `compiler_builtins` are unconditionally compiled with panic=abort to
// workaround undefined references to `rust_eh_unwind_resume` generated
// otherwise, see issue https://github.com/rust-lang/rust/issues/43095.
if crate_name == Some("panic_abort") ||
if (crate_name == Some("panic_abort") && stage == "0") ||
crate_name == Some("compiler_builtins") && stage != "0" {
cmd.arg("-C").arg("panic=abort");
}
Expand Down
1 change: 1 addition & 0 deletions src/libpanic_abort/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#![doc(html_root_url = "https://doc.rust-lang.org/nightly/",
issue_tracker_base_url = "https://github.com/rust-lang/rust/issues/")]
#![panic_runtime]
#![cfg_attr(not(bootstrap), rustc_panic_abort_runtime)]

#![allow(unused_features)]

Expand Down
4 changes: 4 additions & 0 deletions src/librustc_codegen_llvm/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ impl CodegenBackend for LlvmCodegenBackend {
llvm_util::init(sess); // Make sure llvm is inited
}

fn after_expansion(&self, sess: &Session) {
llvm_util::late_init(sess);
}

fn print(&self, req: PrintRequest, sess: &Session) {
match req {
PrintRequest::RelocationModels => {
Expand Down
1 change: 1 addition & 0 deletions src/librustc_codegen_llvm/llvm/ffi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1871,4 +1871,5 @@ extern "C" {
bytecode: *const c_char,
bytecode_len: usize) -> bool;
pub fn LLVMRustLinkerFree(linker: &'a mut Linker<'a>);
pub fn LLVMRustEnableEmscriptenCXXExceptions();
}
12 changes: 7 additions & 5 deletions src/librustc_codegen_llvm/llvm_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ fn require_inited() {
}
}

pub(crate) fn late_init(sess: &Session) {
if sess.target.target.target_os == "emscripten" &&
sess.panic_strategy() == PanicStrategy::Unwind {
unsafe { llvm::LLVMRustEnableEmscriptenCXXExceptions(); }
}
}

unsafe fn configure_llvm(sess: &Session) {
let n_args = sess.opts.cg.llvm_args.len();
let mut llvm_c_strs = Vec::with_capacity(n_args + 1);
Expand Down Expand Up @@ -95,11 +102,6 @@ unsafe fn configure_llvm(sess: &Session) {
}
}

if sess.target.target.target_os == "emscripten" &&
sess.panic_strategy() == PanicStrategy::Unwind {
add("-enable-emscripten-cxx-exceptions", false);
}

// HACK(eddyb) LLVM inserts `llvm.assume` calls to preserve align attributes
// during inlining. Unfortunately these may block other optimizations.
add("-preserve-alignment-assumptions-during-inlining=false", false);
Expand Down
1 change: 1 addition & 0 deletions src/librustc_codegen_utils/codegen_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub use rustc_data_structures::sync::MetadataRef;

pub trait CodegenBackend {
fn init(&self, _sess: &Session) {}
fn after_expansion(&self, _sess: &Session) {}
fn print(&self, _req: PrintRequest, _sess: &Session) {}
fn target_features(&self, _sess: &Session) -> Vec<Symbol> { vec![] }
fn print_passes(&self) {}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_feature/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,7 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
rustc_attr!(rustc_allocator, Whitelisted, template!(Word), IMPL_DETAIL),
rustc_attr!(rustc_allocator_nounwind, Whitelisted, template!(Word), IMPL_DETAIL),
gated!(alloc_error_handler, Normal, template!(Word), experimental!(alloc_error_handler)),
rustc_attr!(rustc_panic_abort_runtime, Whitelisted, template!(Word), IMPL_DETAIL),
gated!(
default_lib_allocator, Whitelisted, template!(Word), allocator_internals,
experimental!(default_lib_allocator),
Expand Down
23 changes: 17 additions & 6 deletions src/librustc_interface/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ use crate::interface::{Compiler, Result};
use crate::util;
use crate::proc_macro_decls;

use log::{info, warn, log_enabled};
use rustc::arena::Arena;
use log::{debug, info, warn, log_enabled};
use rustc::dep_graph::DepGraph;
use rustc::hir;
use rustc::hir::lowering::lower_crate;
Expand Down Expand Up @@ -39,7 +39,7 @@ use syntax::early_buffered_lints::BufferedEarlyLint;
use syntax_expand::base::ExtCtxt;
use syntax::mut_visit::MutVisitor;
use syntax::util::node_count::NodeCounter;
use syntax::symbol::Symbol;
use syntax::symbol::{sym, Symbol};
use syntax_pos::FileName;
use syntax_ext;

Expand Down Expand Up @@ -112,6 +112,7 @@ declare_box_region_type!(
///
/// Returns `None` if we're aborting after handling -W help.
pub fn configure_and_expand(
codegen_backend: Lrc<Box<dyn CodegenBackend>>,
sess: Lrc<Session>,
lint_store: Lrc<lint::LintStore>,
metadata_loader: Box<MetadataLoaderDyn>,
Expand All @@ -123,15 +124,16 @@ pub fn configure_and_expand(
// item, much like we do for macro expansion. In other words, the hash reflects not just
// its contents but the results of name resolution on those contents. Hopefully we'll push
// this back at some point.
let crate_name = crate_name.to_string();
let crate_name_inner = crate_name.to_string();
let sess_inner = sess.clone();
let (result, resolver) = BoxedResolver::new(static move || {
let sess = &*sess;
let resolver_arenas = Resolver::arenas();
let res = configure_and_expand_inner(
sess,
&**codegen_backend,
&sess_inner,
&lint_store,
krate,
&crate_name,
&crate_name_inner,
&resolver_arenas,
&*metadata_loader,
);
Expand Down Expand Up @@ -227,6 +229,7 @@ pub fn register_plugins<'a>(
}

fn configure_and_expand_inner<'a>(
codegen_backend: &dyn CodegenBackend,
sess: &'a Session,
lint_store: &'a lint::LintStore,
mut krate: ast::Crate,
Expand Down Expand Up @@ -346,6 +349,14 @@ fn configure_and_expand_inner<'a>(
krate
});

let force_panic_abort = krate.attrs.iter().any(|attr| {
attr.check_name(sym::rustc_panic_abort_runtime)
});
debug!("Setting force_panic_abort for crate `{}`: {}", crate_name, force_panic_abort);
sess.force_panic_abort.set(force_panic_abort);

codegen_backend.after_expansion(sess);

time(sess, "maybe building test harness", || {
syntax_ext::test_harness::inject(
&sess.parse_sess,
Expand Down
1 change: 1 addition & 0 deletions src/librustc_interface/queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ impl<'tcx> Queries<'tcx> {
let crate_name = self.crate_name()?.peek().clone();
let (krate, lint_store) = self.register_plugins()?.take();
passes::configure_and_expand(
self.codegen_backend().clone(),
self.session().clone(),
lint_store.clone(),
self.codegen_backend().metadata_loader(),
Expand Down
10 changes: 10 additions & 0 deletions src/librustc_session/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ pub struct Session {
/// false positives about a job server in our environment.
pub jobserver: Client,

/// Whether or not to force the panic strategy for this
/// crate to be PanicStrategy::Abort. Only used
/// for 'libpanic_abort'
pub force_panic_abort: Once<bool>,

/// Cap lint level specified by a driver specifically.
pub driver_lint_caps: FxHashMap<lint::LintId, lint::Level>,

Expand Down Expand Up @@ -543,6 +548,10 @@ impl Session {
/// Returns the panic strategy for this compile session. If the user explicitly selected one
/// using '-C panic', use that, otherwise use the panic strategy defined by the target.
pub fn panic_strategy(&self) -> PanicStrategy {
if *self.force_panic_abort.get() {
return PanicStrategy::Abort
}

self.opts
.cg
.panic
Expand Down Expand Up @@ -1164,6 +1173,7 @@ fn build_session_(
print_fuel_crate,
print_fuel,
jobserver: jobserver::client(),
force_panic_abort: Once::new(),
driver_lint_caps,
trait_methods_not_found: Lock::new(Default::default()),
confused_type_with_std_module: Lock::new(Default::default()),
Expand Down
1 change: 1 addition & 0 deletions src/libsyntax_pos/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,7 @@ symbols! {
rustc_object_lifetime_default,
rustc_on_unimplemented,
rustc_outlives,
rustc_panic_abort_runtime,
rustc_paren_sugar,
rustc_partition_codegened,
rustc_partition_reused,
Expand Down
31 changes: 31 additions & 0 deletions src/rustllvm/RustWrapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1537,3 +1537,34 @@ extern "C" LLVMValueRef
LLVMRustBuildMaxNum(LLVMBuilderRef B, LLVMValueRef LHS, LLVMValueRef RHS) {
return wrap(unwrap(B)->CreateMaxNum(unwrap(LHS),unwrap(RHS)));
}

extern "C" void
LLVMRustEnableEmscriptenCXXExceptions() {
// This is a little sketchy. Ideally, we would just pass
// '-enable-emscripten-cxx-exceptions' along with all
// of our other LLVM arguments when we initialize LLVM.
// Unfortunately, whether or not we pass this flag depends
// on the crate panic strategy. Determining the crate
// panic strategy requires us to have paresed crate arributes
// (so that we can have special handling for `libpanic_abort`).
// Parsing crate attributes actually requires us to have initialized
// LLVM, so that we can handle cfg-gating involving LLVM target
// features.
//
// We break this circular dependency by manually enabling
// "enable-emscripten-cxx-exceptions" after we've initialized
// LLVM. The methods involved are not well-documented - however,
// the flag we are modifiying ("enable-emscripten-cxx-exceptions")
// is only used in one place, and only when a PassManger is created.
// Thus, enabling it later than normal should have no visible effects.
//
// If this logic ever becomes incorrect (e.g. due to an LLVM upgrade),
// it should cause panic-related tests to start failing under emscripten,
// since they require this flag for proper unwinding support.
StringMap<cl::Option*> &Map = cl::getRegisteredOptions();
Map["enable-emscripten-cxx-exceptions"]->addOccurrence(
0,
StringRef("enable-emscripten-cxx-exceptions"),
StringRef("")
);
}

0 comments on commit 635c76b

Please sign in to comment.