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

Add a target option "merge-functions", and a corresponding -Z flag (works around #57356) #57268

Merged
merged 1 commit into from
Jan 19, 2019
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: 24 additions & 4 deletions src/librustc/session/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::str::FromStr;
use session::{early_error, early_warn, Session};
use session::search_paths::SearchPath;

use rustc_target::spec::{LinkerFlavor, PanicStrategy, RelroLevel};
use rustc_target::spec::{LinkerFlavor, MergeFunctions, PanicStrategy, RelroLevel};
use rustc_target::spec::{Target, TargetTriple};
use lint;
use middle::cstore;
Expand Down Expand Up @@ -808,13 +808,16 @@ macro_rules! options {
pub const parse_cross_lang_lto: Option<&str> =
Some("either a boolean (`yes`, `no`, `on`, `off`, etc), \
or the path to the linker plugin");
pub const parse_merge_functions: Option<&str> =
Some("one of: `disabled`, `trampolines`, or `aliases`");
}

#[allow(dead_code)]
mod $mod_set {
use super::{$struct_name, Passes, Sanitizer, LtoCli, CrossLangLto};
use rustc_target::spec::{LinkerFlavor, PanicStrategy, RelroLevel};
use rustc_target::spec::{LinkerFlavor, MergeFunctions, PanicStrategy, RelroLevel};
use std::path::PathBuf;
use std::str::FromStr;

$(
pub fn $opt(cg: &mut $struct_name, v: Option<&str>) -> bool {
Expand Down Expand Up @@ -1046,6 +1049,14 @@ macro_rules! options {
};
true
}

fn parse_merge_functions(slot: &mut Option<MergeFunctions>, v: Option<&str>) -> bool {
match v.and_then(|s| MergeFunctions::from_str(s).ok()) {
Some(mergefunc) => *slot = Some(mergefunc),
_ => return false,
}
true
}
}
) }

Expand Down Expand Up @@ -1380,6 +1391,9 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"whether to use the PLT when calling into shared libraries;
only has effect for PIC code on systems with ELF binaries
(default: PLT is disabled if full relro is enabled)"),
merge_functions: Option<MergeFunctions> = (None, parse_merge_functions, [TRACKED],
"control the operation of the MergeFunctions LLVM pass, taking
the same values as the target option of the same name"),
Copy link
Member

Choose a reason for hiding this comment

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

This is going to cause an ICE later when invalid values are provided as an argument, because argument parsing does not verify the valid values. Consider validating this in argument parsing.

fn parse_panic_strategy(slot: &mut Option<PanicStrategy>, v: Option<&str>) -> bool {
match v {
Some("unwind") => *slot = Some(PanicStrategy::Unwind),
Some("abort") => *slot = Some(PanicStrategy::Abort),
_ => return false
}
true
}

is a good example of how arguments should be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with a MergeFunctions enum

}

pub fn default_lib_output() -> CrateType {
Expand Down Expand Up @@ -2398,7 +2412,7 @@ mod dep_tracking {
use super::{CrateType, DebugInfo, ErrorOutputType, OptLevel, OutputTypes,
Passes, Sanitizer, LtoCli, CrossLangLto};
use syntax::feature_gate::UnstableFeatures;
use rustc_target::spec::{PanicStrategy, RelroLevel, TargetTriple};
use rustc_target::spec::{MergeFunctions, PanicStrategy, RelroLevel, TargetTriple};
use syntax::edition::Edition;

pub trait DepTrackingHash {
Expand Down Expand Up @@ -2441,12 +2455,14 @@ mod dep_tracking {
impl_dep_tracking_hash_via_hash!(Option<usize>);
impl_dep_tracking_hash_via_hash!(Option<String>);
impl_dep_tracking_hash_via_hash!(Option<(String, u64)>);
impl_dep_tracking_hash_via_hash!(Option<MergeFunctions>);
impl_dep_tracking_hash_via_hash!(Option<PanicStrategy>);
impl_dep_tracking_hash_via_hash!(Option<RelroLevel>);
impl_dep_tracking_hash_via_hash!(Option<lint::Level>);
impl_dep_tracking_hash_via_hash!(Option<PathBuf>);
impl_dep_tracking_hash_via_hash!(Option<cstore::NativeLibraryKind>);
impl_dep_tracking_hash_via_hash!(CrateType);
impl_dep_tracking_hash_via_hash!(MergeFunctions);
impl_dep_tracking_hash_via_hash!(PanicStrategy);
impl_dep_tracking_hash_via_hash!(RelroLevel);
impl_dep_tracking_hash_via_hash!(Passes);
Expand Down Expand Up @@ -2532,7 +2548,7 @@ mod tests {
use std::iter::FromIterator;
use std::path::PathBuf;
use super::{Externs, OutputType, OutputTypes};
use rustc_target::spec::{PanicStrategy, RelroLevel};
use rustc_target::spec::{MergeFunctions, PanicStrategy, RelroLevel};
use syntax::symbol::Symbol;
use syntax::edition::{Edition, DEFAULT_EDITION};
use syntax;
Expand Down Expand Up @@ -3187,6 +3203,10 @@ mod tests {
opts = reference.clone();
opts.debugging_opts.cross_lang_lto = CrossLangLto::LinkerPluginAuto;
assert!(reference.dep_tracking_hash() != opts.dep_tracking_hash());

opts = reference.clone();
opts.debugging_opts.merge_functions = Some(MergeFunctions::Disabled);
assert!(reference.dep_tracking_hash() != opts.dep_tracking_hash());
}

#[test]
Expand Down
10 changes: 9 additions & 1 deletion src/librustc_codegen_llvm/llvm_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use back::write::create_target_machine;
use llvm;
use rustc::session::Session;
use rustc::session::config::PrintRequest;
use rustc_target::spec::MergeFunctions;
use libc::c_int;
use std::ffi::CString;
use syntax::feature_gate::UnstableFeatures;
Expand Down Expand Up @@ -61,7 +62,14 @@ unsafe fn configure_llvm(sess: &Session) {
add("-disable-preinline");
}
if llvm::LLVMRustIsRustLLVM() {
add("-mergefunc-use-aliases");
match sess.opts.debugging_opts.merge_functions
.unwrap_or(sess.target.target.options.merge_functions) {
MergeFunctions::Disabled |
MergeFunctions::Trampolines => {}
MergeFunctions::Aliases => {
add("-mergefunc-use-aliases");
}
}
}

// HACK(eddyb) LLVM inserts `llvm.assume` calls to preserve align attributes
Expand Down
21 changes: 19 additions & 2 deletions src/librustc_codegen_ssa/back/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use rustc_fs_util::link_or_copy;
use rustc_data_structures::svh::Svh;
use rustc_errors::{Handler, Level, DiagnosticBuilder, FatalError, DiagnosticId};
use rustc_errors::emitter::{Emitter};
use rustc_target::spec::MergeFunctions;
use syntax::attr;
use syntax::ext::hygiene::Mark;
use syntax_pos::MultiSpan;
Expand Down Expand Up @@ -152,8 +153,24 @@ impl ModuleConfig {
sess.opts.optimize == config::OptLevel::Aggressive &&
!sess.target.target.options.is_like_emscripten;

self.merge_functions = sess.opts.optimize == config::OptLevel::Default ||
sess.opts.optimize == config::OptLevel::Aggressive;
// Some targets (namely, NVPTX) interact badly with the MergeFunctions
// pass. This is because MergeFunctions can generate new function calls
// which may interfere with the target calling convention; e.g. for the
// NVPTX target, PTX kernels should not call other PTX kernels.
// MergeFunctions can also be configured to generate aliases instead,
// but aliases are not supported by some backends (again, NVPTX).
// Therefore, allow targets to opt out of the MergeFunctions pass,
// but otherwise keep the pass enabled (at O2 and O3) since it can be
// useful for reducing code size.
self.merge_functions = match sess.opts.debugging_opts.merge_functions
.unwrap_or(sess.target.target.options.merge_functions) {
MergeFunctions::Disabled => false,
MergeFunctions::Trampolines |
MergeFunctions::Aliases => {
sess.opts.optimize == config::OptLevel::Default ||
sess.opts.optimize == config::OptLevel::Aggressive
}
};
}

pub fn bitcode_needed(&self) -> bool {
Expand Down
66 changes: 65 additions & 1 deletion src/librustc_target/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,46 @@ impl ToJson for RelroLevel {
}
}

#[derive(Clone, Copy, Debug, PartialEq, Hash, RustcEncodable, RustcDecodable)]
pub enum MergeFunctions {
Disabled,
Trampolines,
Aliases
}

impl MergeFunctions {
pub fn desc(&self) -> &str {
match *self {
MergeFunctions::Disabled => "disabled",
MergeFunctions::Trampolines => "trampolines",
MergeFunctions::Aliases => "aliases",
}
}
}

impl FromStr for MergeFunctions {
type Err = ();

fn from_str(s: &str) -> Result<MergeFunctions, ()> {
match s {
"disabled" => Ok(MergeFunctions::Disabled),
"trampolines" => Ok(MergeFunctions::Trampolines),
"aliases" => Ok(MergeFunctions::Aliases),
_ => Err(()),
}
}
}

impl ToJson for MergeFunctions {
fn to_json(&self) -> Json {
match *self {
MergeFunctions::Disabled => "disabled".to_json(),
MergeFunctions::Trampolines => "trampolines".to_json(),
MergeFunctions::Aliases => "aliases".to_json(),
}
}
}

pub type LinkArgs = BTreeMap<LinkerFlavor, Vec<String>>;
pub type TargetResult = Result<Target, String>;

Expand Down Expand Up @@ -690,7 +730,15 @@ pub struct TargetOptions {

/// If set, have the linker export exactly these symbols, instead of using
/// the usual logic to figure this out from the crate itself.
pub override_export_symbols: Option<Vec<String>>
pub override_export_symbols: Option<Vec<String>>,

/// Determines how or whether the MergeFunctions LLVM pass should run for
/// this target. Either "disabled", "trampolines", or "aliases".
/// The MergeFunctions pass is generally useful, but some targets may need
/// to opt out. The default is "aliases".
///
/// Workaround for: https://github.com/rust-lang/rust/issues/57356
pub merge_functions: MergeFunctions
}

impl Default for TargetOptions {
Expand Down Expand Up @@ -773,6 +821,7 @@ impl Default for TargetOptions {
requires_uwtable: false,
simd_types_indirect: true,
override_export_symbols: None,
merge_functions: MergeFunctions::Aliases,
}
}
}
Expand Down Expand Up @@ -875,6 +924,19 @@ impl Target {
.map(|o| o.as_u64()
.map(|s| base.options.$key_name = Some(s)));
} );
($key_name:ident, MergeFunctions) => ( {
let name = (stringify!($key_name)).replace("_", "-");
obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| {
match s.parse::<MergeFunctions>() {
Ok(mergefunc) => base.options.$key_name = mergefunc,
_ => return Some(Err(format!("'{}' is not a valid value for \
merge-functions. Use 'disabled', \
'trampolines', or 'aliases'.",
s))),
}
Some(Ok(()))
})).unwrap_or(Ok(()))
} );
($key_name:ident, PanicStrategy) => ( {
let name = (stringify!($key_name)).replace("_", "-");
obj.find(&name[..]).and_then(|o| o.as_string().and_then(|s| {
Expand Down Expand Up @@ -1064,6 +1126,7 @@ impl Target {
key!(requires_uwtable, bool);
key!(simd_types_indirect, bool);
key!(override_export_symbols, opt_list);
key!(merge_functions, MergeFunctions)?;

if let Some(array) = obj.find("abi-blacklist").and_then(Json::as_array) {
for name in array.iter().filter_map(|abi| abi.as_string()) {
Expand Down Expand Up @@ -1275,6 +1338,7 @@ impl ToJson for Target {
target_option_val!(requires_uwtable);
target_option_val!(simd_types_indirect);
target_option_val!(override_export_symbols);
target_option_val!(merge_functions);

if default.abi_blacklist != self.options.abi_blacklist {
d.insert("abi-blacklist".to_string(), self.options.abi_blacklist.iter()
Expand Down