From 8fda46477a724fb163ce1759917d77b76be041b0 Mon Sep 17 00:00:00 2001 From: Robin Kruppe Date: Fri, 12 Jan 2018 23:32:14 +0100 Subject: [PATCH 1/5] Compute LLVM argument indices correctly in face of padding Closes #47278 --- src/librustc_trans/abi.rs | 3 --- src/librustc_trans/mir/mod.rs | 3 +++ src/test/codegen/issue-47278.rs | 19 +++++++++++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 src/test/codegen/issue-47278.rs diff --git a/src/librustc_trans/abi.rs b/src/librustc_trans/abi.rs index 32dc1067d37c1..045ce6efd64b9 100644 --- a/src/librustc_trans/abi.rs +++ b/src/librustc_trans/abi.rs @@ -608,9 +608,6 @@ impl<'a, 'tcx> ArgType<'tcx> { } pub fn store_fn_arg(&self, bcx: &Builder<'a, 'tcx>, idx: &mut usize, dst: PlaceRef<'tcx>) { - if self.pad.is_some() { - *idx += 1; - } let mut next = || { let val = llvm::get_param(bcx.llfn(), *idx as c_uint); *idx += 1; diff --git a/src/librustc_trans/mir/mod.rs b/src/librustc_trans/mir/mod.rs index 917ff87a674b6..0ac845a2fa241 100644 --- a/src/librustc_trans/mir/mod.rs +++ b/src/librustc_trans/mir/mod.rs @@ -402,6 +402,9 @@ fn arg_local_refs<'a, 'tcx>(bcx: &Builder<'a, 'tcx>, for i in 0..tupled_arg_tys.len() { let arg = &mircx.fn_ty.args[idx]; idx += 1; + if arg.pad.is_some() { + llarg_idx += 1; + } arg.store_fn_arg(bcx, &mut llarg_idx, place.project_field(bcx, i)); } diff --git a/src/test/codegen/issue-47278.rs b/src/test/codegen/issue-47278.rs new file mode 100644 index 0000000000000..21858b434bf19 --- /dev/null +++ b/src/test/codegen/issue-47278.rs @@ -0,0 +1,19 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// -C no-prepopulate-passes +#![crate_type="staticlib"] + +#[repr(C)] +pub struct Foo(u64); + +// CHECK: define {{.*}} @foo( +#[no_mangle] +pub extern fn foo(_: Foo) -> Foo { loop {} } From 812d4c58009ed7aebd7e1bef3d7d5374bd4d496a Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Tue, 16 Jan 2018 17:30:11 +0100 Subject: [PATCH 2/5] Don't include DefIndex in plugin- and proc-macro registrar fn symbol. --- src/librustc/session/mod.rs | 16 +++++++--------- src/librustc_metadata/creader.rs | 16 +++++++--------- src/librustc_plugin/load.rs | 4 ++-- src/librustc_trans/back/symbol_export.rs | 3 +-- src/librustc_trans/back/symbol_names.rs | 6 ++---- 5 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/librustc/session/mod.rs b/src/librustc/session/mod.rs index 5f580bc2151db..75dc2d4e8e688 100644 --- a/src/librustc/session/mod.rs +++ b/src/librustc/session/mod.rs @@ -11,7 +11,7 @@ pub use self::code_stats::{CodeStats, DataTypeKind, FieldInfo}; pub use self::code_stats::{SizeKind, TypeSizeInfo, VariantInfo}; -use hir::def_id::{CrateNum, DefIndex}; +use hir::def_id::CrateNum; use ich::Fingerprint; use lint; @@ -558,18 +558,16 @@ impl Session { /// Returns the symbol name for the registrar function, /// given the crate Svh and the function DefIndex. - pub fn generate_plugin_registrar_symbol(&self, disambiguator: CrateDisambiguator, - index: DefIndex) + pub fn generate_plugin_registrar_symbol(&self, + disambiguator: CrateDisambiguator) -> String { - format!("__rustc_plugin_registrar__{}_{}", disambiguator.to_fingerprint().to_hex(), - index.as_usize()) + format!("__rustc_plugin_registrar_{}__", disambiguator.to_fingerprint().to_hex()) } - pub fn generate_derive_registrar_symbol(&self, disambiguator: CrateDisambiguator, - index: DefIndex) + pub fn generate_derive_registrar_symbol(&self, + disambiguator: CrateDisambiguator) -> String { - format!("__rustc_derive_registrar__{}_{}", disambiguator.to_fingerprint().to_hex(), - index.as_usize()) + format!("__rustc_derive_registrar_{}__", disambiguator.to_fingerprint().to_hex()) } pub fn sysroot<'a>(&'a self) -> &'a Path { diff --git a/src/librustc_metadata/creader.rs b/src/librustc_metadata/creader.rs index 946eecaa45f7d..246f5c9255ef4 100644 --- a/src/librustc_metadata/creader.rs +++ b/src/librustc_metadata/creader.rs @@ -15,7 +15,7 @@ use locator::{self, CratePaths}; use native_libs::relevant_lib; use schema::CrateRoot; -use rustc::hir::def_id::{CrateNum, DefIndex, CRATE_DEF_INDEX}; +use rustc::hir::def_id::{CrateNum, CRATE_DEF_INDEX}; use rustc::hir::svh::Svh; use rustc::middle::allocator::AllocatorKind; use rustc::middle::cstore::DepKind; @@ -532,8 +532,7 @@ impl<'a> CrateLoader<'a> { Err(err) => self.sess.span_fatal(span, &err), }; - let sym = self.sess.generate_derive_registrar_symbol(root.disambiguator, - root.macro_derive_registrar.unwrap()); + let sym = self.sess.generate_derive_registrar_symbol(root.disambiguator); let registrar = unsafe { let sym = match lib.symbol(&sym) { Ok(f) => f, @@ -588,7 +587,7 @@ impl<'a> CrateLoader<'a> { pub fn find_plugin_registrar(&mut self, span: Span, name: &str) - -> Option<(PathBuf, CrateDisambiguator, DefIndex)> { + -> Option<(PathBuf, CrateDisambiguator)> { let name = Symbol::intern(name); let ekrate = self.read_extension_crate(span, name, name); @@ -603,11 +602,11 @@ impl<'a> CrateLoader<'a> { } let root = ekrate.metadata.get_root(); - match (ekrate.dylib.as_ref(), root.plugin_registrar_fn) { - (Some(dylib), Some(reg)) => { - Some((dylib.to_path_buf(), root.disambiguator, reg)) + match ekrate.dylib.as_ref() { + Some(dylib) => { + Some((dylib.to_path_buf(), root.disambiguator)) } - (None, Some(_)) => { + None => { span_err!(self.sess, span, E0457, "plugin `{}` only found in rlib format, but must be available \ in dylib format", @@ -616,7 +615,6 @@ impl<'a> CrateLoader<'a> { // empty dylib. None } - _ => None, } } diff --git a/src/librustc_plugin/load.rs b/src/librustc_plugin/load.rs index 8a4ec03b20efc..a46b85d93cbb8 100644 --- a/src/librustc_plugin/load.rs +++ b/src/librustc_plugin/load.rs @@ -100,8 +100,8 @@ impl<'a> PluginLoader<'a> { fn load_plugin(&mut self, span: Span, name: &str, args: Vec) { let registrar = self.reader.find_plugin_registrar(span, name); - if let Some((lib, disambiguator, index)) = registrar { - let symbol = self.sess.generate_plugin_registrar_symbol(disambiguator, index); + if let Some((lib, disambiguator)) = registrar { + let symbol = self.sess.generate_plugin_registrar_symbol(disambiguator); let fun = self.dylink_registrar(span, lib, symbol); self.plugins.push(PluginRegistrar { fun, diff --git a/src/librustc_trans/back/symbol_export.rs b/src/librustc_trans/back/symbol_export.rs index fa6fe2e9e93ef..15ff59c7df998 100644 --- a/src/librustc_trans/back/symbol_export.rs +++ b/src/librustc_trans/back/symbol_export.rs @@ -115,9 +115,8 @@ pub fn provide(providers: &mut Providers) { if let Some(id) = tcx.sess.derive_registrar_fn.get() { let def_id = tcx.hir.local_def_id(id); - let idx = def_id.index; let disambiguator = tcx.sess.local_crate_disambiguator(); - let registrar = tcx.sess.generate_derive_registrar_symbol(disambiguator, idx); + let registrar = tcx.sess.generate_derive_registrar_symbol(disambiguator); local_crate.push((registrar, Some(def_id), SymbolExportLevel::C)); } diff --git a/src/librustc_trans/back/symbol_names.rs b/src/librustc_trans/back/symbol_names.rs index 825f306499a7c..3ceff659ea93a 100644 --- a/src/librustc_trans/back/symbol_names.rs +++ b/src/librustc_trans/back/symbol_names.rs @@ -257,14 +257,12 @@ fn compute_symbol_name<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, instance: Instance if let Some(id) = node_id { if tcx.sess.plugin_registrar_fn.get() == Some(id) { - let idx = def_id.index; let disambiguator = tcx.sess.local_crate_disambiguator(); - return tcx.sess.generate_plugin_registrar_symbol(disambiguator, idx); + return tcx.sess.generate_plugin_registrar_symbol(disambiguator); } if tcx.sess.derive_registrar_fn.get() == Some(id) { - let idx = def_id.index; let disambiguator = tcx.sess.local_crate_disambiguator(); - return tcx.sess.generate_derive_registrar_symbol(disambiguator, idx); + return tcx.sess.generate_derive_registrar_symbol(disambiguator); } } From dc957614090897b4899b7585e5e6a8e0c709871c Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Tue, 16 Jan 2018 19:31:15 +0100 Subject: [PATCH 3/5] Add incremental-fulldeps test suite and regression test for #47290. --- src/bootstrap/check.rs | 5 +++ .../auxiliary/incremental_proc_macro_aux.rs | 31 +++++++++++++++++++ .../incremental_proc_macro.rs | 27 ++++++++++++++++ 3 files changed, 63 insertions(+) create mode 100644 src/test/incremental-fulldeps/auxiliary/incremental_proc_macro_aux.rs create mode 100644 src/test/incremental-fulldeps/incremental_proc_macro.rs diff --git a/src/bootstrap/check.rs b/src/bootstrap/check.rs index 714c01aa50446..48b3d356985c3 100644 --- a/src/bootstrap/check.rs +++ b/src/bootstrap/check.rs @@ -568,6 +568,11 @@ static HOST_COMPILETESTS: &[Test] = &[ mode: "compile-fail", suite: "compile-fail-fulldeps", }, + Test { + path: "src/test/incremental-fulldeps", + mode: "incremental", + suite: "incremental-fulldeps", + }, Test { path: "src/test/run-make", mode: "run-make", suite: "run-make" }, Test { path: "src/test/rustdoc", mode: "rustdoc", suite: "rustdoc" }, diff --git a/src/test/incremental-fulldeps/auxiliary/incremental_proc_macro_aux.rs b/src/test/incremental-fulldeps/auxiliary/incremental_proc_macro_aux.rs new file mode 100644 index 0000000000000..e9f9ba86f23dc --- /dev/null +++ b/src/test/incremental-fulldeps/auxiliary/incremental_proc_macro_aux.rs @@ -0,0 +1,31 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// no-prefer-dynamic + +#![crate_type = "proc-macro"] + +extern crate proc_macro; + +use proc_macro::TokenStream; + +// Add a function to shift DefIndex of registrar function +#[cfg(cfail2)] +fn foo() {} + +#[proc_macro_derive(IncrementalMacro)] +pub fn derive(input: TokenStream) -> TokenStream { + #[cfg(cfail2)] + { + foo(); + } + + "".parse().unwrap() +} diff --git a/src/test/incremental-fulldeps/incremental_proc_macro.rs b/src/test/incremental-fulldeps/incremental_proc_macro.rs new file mode 100644 index 0000000000000..e434507085332 --- /dev/null +++ b/src/test/incremental-fulldeps/incremental_proc_macro.rs @@ -0,0 +1,27 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// aux-build:incremental_proc_macro_aux.rs +// ignore-stage1 +// revisions: cfail1 cfail2 +// must-compile-successfully + +// This test makes sure that we still find the proc-macro registrar function +// when we compile proc-macros incrementally (see #47292). + +#![crate_type = "rlib"] + +#[macro_use] +extern crate incremental_proc_macro_aux; + +#[derive(IncrementalMacro)] +pub struct Foo { + x: u32 +} From 7973dc9d128bc61b4151ff4ff50ae8b531bc7db7 Mon Sep 17 00:00:00 2001 From: Ariel Ben-Yehuda Date: Tue, 16 Jan 2018 23:05:13 +0200 Subject: [PATCH 4/5] avoid double-unsizing arrays in bytestring match lowering The match lowering code, when lowering matches against bytestrings, works by coercing both the scrutinee and the pattern to `&[u8]` and then comparing them using `<[u8] as Eq>::eq`. If the scrutinee is already of type `&[u8]`, then unsizing it is both unneccessary and a trait error caught by the new and updated MIR typeck, so this PR changes lowering to avoid doing that (match lowering tried to avoid that before, but that attempt was quite broken). Fixes #46920. --- src/librustc_mir/build/matches/test.rs | 74 +++++++++++++------ .../issue-46920-byte-array-patterns.rs | 37 ++++++++++ 2 files changed, 88 insertions(+), 23 deletions(-) create mode 100644 src/test/run-pass/issue-46920-byte-array-patterns.rs diff --git a/src/librustc_mir/build/matches/test.rs b/src/librustc_mir/build/matches/test.rs index b2357b771572f..bdcbfc0bdd85e 100644 --- a/src/librustc_mir/build/matches/test.rs +++ b/src/librustc_mir/build/matches/test.rs @@ -174,12 +174,50 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } + /// Convert a byte array or byte slice to a byte slice. + fn to_slice_operand(&mut self, + block: BasicBlock, + source_info: SourceInfo, + operand: Operand<'tcx>) + -> Operand<'tcx> + { + let tcx = self.hir.tcx(); + let ty = operand.ty(&self.local_decls, tcx); + debug!("to_slice_operand({:?}, {:?}: {:?})", block, operand, ty); + match ty.sty { + ty::TyRef(region, mt) => match mt.ty.sty { + ty::TyArray(ety, _) => { + let ty = tcx.mk_imm_ref(region, tcx.mk_slice(ety)); + let temp = self.temp(ty, source_info.span); + self.cfg.push_assign(block, source_info, &temp, + Rvalue::Cast(CastKind::Unsize, operand, ty)); + Operand::Move(temp) + } + ty::TySlice(_) => operand, + _ => { + span_bug!(source_info.span, + "bad operand {:?}: {:?} to `to_slice_operand`", operand, ty) + } + } + _ => { + span_bug!(source_info.span, + "bad operand {:?}: {:?} to `to_slice_operand`", operand, ty) + } + } + + } + /// Generates the code to perform a test. pub fn perform_test(&mut self, block: BasicBlock, place: &Place<'tcx>, test: &Test<'tcx>) -> Vec { + debug!("perform_test({:?}, {:?}: {:?}, {:?})", + block, + place, + place.ty(&self.local_decls, self.hir.tcx()), + test); let source_info = self.source_info(test.span); match test.kind { TestKind::Switch { adt_def, ref variants } => { @@ -258,45 +296,35 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ret } - TestKind::Eq { value, mut ty } => { + TestKind::Eq { value, ty } => { + let tcx = self.hir.tcx(); let mut val = Operand::Copy(place.clone()); // If we're using b"..." as a pattern, we need to insert an // unsizing coercion, as the byte string has the type &[u8; N]. - let expect = if let ConstVal::ByteStr(bytes) = value.val { - let tcx = self.hir.tcx(); - - // Unsize the place to &[u8], too, if necessary. - if let ty::TyRef(region, mt) = ty.sty { - if let ty::TyArray(_, _) = mt.ty.sty { - ty = tcx.mk_imm_ref(region, tcx.mk_slice(tcx.types.u8)); - let val_slice = self.temp(ty, test.span); - self.cfg.push_assign(block, source_info, &val_slice, - Rvalue::Cast(CastKind::Unsize, val, ty)); - val = Operand::Move(val_slice); - } - } - - assert!(ty.is_slice()); - + // + // We want to do this even when the scrutinee is a reference to an + // array, so we can call `<[u8]>::eq` rather than having to find an + // `<[u8; N]>::eq`. + let (expect, val) = if let ConstVal::ByteStr(bytes) = value.val { let array_ty = tcx.mk_array(tcx.types.u8, bytes.data.len() as u64); let array_ref = tcx.mk_imm_ref(tcx.types.re_static, array_ty); let array = self.literal_operand(test.span, array_ref, Literal::Value { value }); - let slice = self.temp(ty, test.span); - self.cfg.push_assign(block, source_info, &slice, - Rvalue::Cast(CastKind::Unsize, array, ty)); - Operand::Move(slice) + let val = self.to_slice_operand(block, source_info, val); + let slice = self.to_slice_operand(block, source_info, array); + (slice, val) } else { - self.literal_operand(test.span, ty, Literal::Value { + (self.literal_operand(test.span, ty, Literal::Value { value - }) + }), val) }; // Use PartialEq::eq for &str and &[u8] slices, instead of BinOp::Eq. let fail = self.cfg.start_new_block(); + let ty = expect.ty(&self.local_decls, tcx); if let ty::TyRef(_, mt) = ty.sty { assert!(ty.is_slice()); let eq_def_id = self.hir.tcx().lang_items().eq_trait().unwrap(); diff --git a/src/test/run-pass/issue-46920-byte-array-patterns.rs b/src/test/run-pass/issue-46920-byte-array-patterns.rs new file mode 100644 index 0000000000000..236f6995c51b8 --- /dev/null +++ b/src/test/run-pass/issue-46920-byte-array-patterns.rs @@ -0,0 +1,37 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +const CURSOR_PARTITION_LABEL: &'static [u8] = b"partition"; +const CURSOR_EVENT_TYPE_LABEL: &'static [u8] = b"event_type"; +const BYTE_PATTERN: &'static [u8; 5] = b"hello"; + +fn match_slice(x: &[u8]) -> u32 { + match x { + CURSOR_PARTITION_LABEL => 0, + CURSOR_EVENT_TYPE_LABEL => 1, + _ => 2, + } +} + +fn match_array(x: &[u8; 5]) -> bool { + match x { + BYTE_PATTERN => true, + _ => false + } +} + +fn main() { + assert_eq!(match_slice(b"abcde"), 2); + assert_eq!(match_slice(b"event_type"), 1); + assert_eq!(match_slice(b"partition"), 0); + + assert_eq!(match_array(b"hello"), true); + assert_eq!(match_array(b"hella"), false); +} From 590924b63542bd2af9f6dc0319dfb1d0c04eafde Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 17 Jan 2018 00:30:57 +0100 Subject: [PATCH 5/5] rustc: Lower link args to `@`-files on Windows more When spawning a linker rustc has historically been known to blow OS limits for the command line being too large, notably on Windows. This is especially true of incremental compilation where there can be dozens of object files per compilation. The compiler currently has logic for detecting a failure to spawn and instead passing arguments via a file instead, but this failure detection only triggers if a process actually fails to spawn. Unfortunately on Windows we've got something else to worry about which is `cmd.exe`. The compiler may be running a linker through `cmd.exe` where `cmd.exe` has a limit of 8192 on the command line vs 32k on `CreateProcess`. Moreso rustc actually succeeds in spawning `cmd.exe` today, it's just that after it's running `cmd.exe` fails to spawn its child, which rustc doesn't currently detect. Consequently this commit updates the logic for the spawning the linker on Windows to instead have a heuristic to see if we need to pass arguments via a file. This heuristic is an overly pessimistic and "inaccurate" calculation which just calls `len` on a bunch of `OsString` instances (where `len` is not precisely the length in u16 elements). This number, when exceeding the 6k threshold, will force rustc to always pass arguments through a file. This strategy should avoid us trying to parse the output on Windows of the linker to see if it successfully spawned yet failed to actually sub-spawn the linker. We may just be passing arguments through files a little more commonly now... The motivation for this commit was a recent bug in Gecko [1] when beta testing, notably when incremental compilation was enabled it blew out the limit on `cmd.exe`. This commit will also fix #46999 as well though as emscripten uses a bat script as well (and we're blowing the limit there). [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1430886 Closes #46999 (cherry picked from commit 66366f9) --- src/librustc_trans/back/command.rs | 72 +++++++++++--- src/librustc_trans/back/link.rs | 38 ++++---- src/librustc_trans/lib.rs | 1 + .../Makefile | 6 ++ .../long-linker-command-lines-cmd-exe/foo.bat | 1 + .../long-linker-command-lines-cmd-exe/foo.rs | 96 +++++++++++++++++++ 6 files changed, 182 insertions(+), 32 deletions(-) create mode 100644 src/test/run-make/long-linker-command-lines-cmd-exe/Makefile create mode 100644 src/test/run-make/long-linker-command-lines-cmd-exe/foo.bat create mode 100644 src/test/run-make/long-linker-command-lines-cmd-exe/foo.rs diff --git a/src/librustc_trans/back/command.rs b/src/librustc_trans/back/command.rs index ea68e3b28b668..05489807fa614 100644 --- a/src/librustc_trans/back/command.rs +++ b/src/librustc_trans/back/command.rs @@ -14,22 +14,34 @@ use std::ffi::{OsStr, OsString}; use std::fmt; use std::io; +use std::mem; use std::process::{self, Output, Child}; +#[derive(Clone)] pub struct Command { - program: OsString, + program: Program, args: Vec, env: Vec<(OsString, OsString)>, } +#[derive(Clone)] +enum Program { + Normal(OsString), + CmdBatScript(OsString), +} + impl Command { pub fn new>(program: P) -> Command { - Command::_new(program.as_ref()) + Command::_new(Program::Normal(program.as_ref().to_owned())) + } + + pub fn bat_script>(program: P) -> Command { + Command::_new(Program::CmdBatScript(program.as_ref().to_owned())) } - fn _new(program: &OsStr) -> Command { + fn _new(program: Program) -> Command { Command { - program: program.to_owned(), + program, args: Vec::new(), env: Vec::new(), } @@ -86,7 +98,14 @@ impl Command { } pub fn command(&self) -> process::Command { - let mut ret = process::Command::new(&self.program); + let mut ret = match self.program { + Program::Normal(ref p) => process::Command::new(p), + Program::CmdBatScript(ref p) => { + let mut c = process::Command::new("cmd"); + c.arg("/c").arg(p); + c + } + }; ret.args(&self.args); ret.envs(self.env.clone()); return ret @@ -94,16 +113,45 @@ impl Command { // extensions - pub fn get_program(&self) -> &OsStr { - &self.program + pub fn take_args(&mut self) -> Vec { + mem::replace(&mut self.args, Vec::new()) } - pub fn get_args(&self) -> &[OsString] { - &self.args - } + /// Returns a `true` if we're pretty sure that this'll blow OS spawn limits, + /// or `false` if we should attempt to spawn and see what the OS says. + pub fn very_likely_to_exceed_some_spawn_limit(&self) -> bool { + // We mostly only care about Windows in this method, on Unix the limits + // can be gargantuan anyway so we're pretty unlikely to hit them + if cfg!(unix) { + return false + } - pub fn get_env(&self) -> &[(OsString, OsString)] { - &self.env + // Ok so on Windows to spawn a process is 32,768 characters in its + // command line [1]. Unfortunately we don't actually have access to that + // as it's calculated just before spawning. Instead we perform a + // poor-man's guess as to how long our command line will be. We're + // assuming here that we don't have to escape every character... + // + // Turns out though that `cmd.exe` has even smaller limits, 8192 + // characters [2]. Linkers can often be batch scripts (for example + // Emscripten, Gecko's current build system) which means that we're + // running through batch scripts. These linkers often just forward + // arguments elsewhere (and maybe tack on more), so if we blow 8192 + // bytes we'll typically cause them to blow as well. + // + // Basically as a result just perform an inflated estimate of what our + // command line will look like and test if it's > 8192 (we actually + // test against 6k to artificially inflate our estimate). If all else + // fails we'll fall back to the normal unix logic of testing the OS + // error code if we fail to spawn and automatically re-spawning the + // linker with smaller arguments. + // + // [1]: https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx + // [2]: https://blogs.msdn.microsoft.com/oldnewthing/20031210-00/?p=41553 + + let estimated_command_line_len = + self.args.iter().map(|a| a.len()).sum::(); + estimated_command_line_len > 1024 * 6 } } diff --git a/src/librustc_trans/back/link.rs b/src/librustc_trans/back/link.rs index 896acad6754e9..360f85d78f172 100644 --- a/src/librustc_trans/back/link.rs +++ b/src/librustc_trans/back/link.rs @@ -36,8 +36,8 @@ use std::char; use std::env; use std::ffi::OsString; use std::fmt; -use std::fs::{self, File}; -use std::io::{self, Write, BufWriter}; +use std::fs; +use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::process::{Output, Stdio}; use std::str; @@ -71,9 +71,7 @@ pub fn get_linker(sess: &Session) -> (PathBuf, Command, Vec<(OsString, OsString) let cmd = |linker: &Path| { if let Some(linker) = linker.to_str() { if cfg!(windows) && linker.ends_with(".bat") { - let mut cmd = Command::new("cmd"); - cmd.arg("/c").arg(linker); - return cmd + return Command::bat_script(linker) } } Command::new(linker) @@ -760,26 +758,26 @@ fn exec_linker(sess: &Session, cmd: &mut Command, tmpdir: &Path) // that contains all the arguments. The theory is that this is then // accepted on all linkers and the linker will read all its options out of // there instead of looking at the command line. - match cmd.command().stdout(Stdio::piped()).stderr(Stdio::piped()).spawn() { - Ok(child) => return child.wait_with_output(), - Err(ref e) if command_line_too_big(e) => {} - Err(e) => return Err(e) + if !cmd.very_likely_to_exceed_some_spawn_limit() { + match cmd.command().stdout(Stdio::piped()).stderr(Stdio::piped()).spawn() { + Ok(child) => return child.wait_with_output(), + Err(ref e) if command_line_too_big(e) => {} + Err(e) => return Err(e) + } } - let file = tmpdir.join("linker-arguments"); - let mut cmd2 = Command::new(cmd.get_program()); - cmd2.arg(format!("@{}", file.display())); - for &(ref k, ref v) in cmd.get_env() { - cmd2.env(k, v); - } - let mut f = BufWriter::new(File::create(&file)?); - for arg in cmd.get_args() { - writeln!(f, "{}", Escape { + let mut cmd2 = cmd.clone(); + let mut args = String::new(); + for arg in cmd2.take_args() { + args.push_str(&Escape { arg: arg.to_str().unwrap(), is_like_msvc: sess.target.target.options.is_like_msvc, - })?; + }.to_string()); + args.push_str("\n"); } - f.into_inner()?; + let file = tmpdir.join("linker-arguments"); + fs::write(&file, args.as_bytes())?; + cmd2.arg(format!("@{}", file.display())); return cmd2.output(); #[cfg(unix)] diff --git a/src/librustc_trans/lib.rs b/src/librustc_trans/lib.rs index c4849c621e8d5..cd02505267ede 100644 --- a/src/librustc_trans/lib.rs +++ b/src/librustc_trans/lib.rs @@ -22,6 +22,7 @@ #![feature(box_patterns)] #![feature(box_syntax)] #![feature(custom_attribute)] +#![feature(fs_read_write)] #![allow(unused_attributes)] #![feature(i128_type)] #![feature(i128)] diff --git a/src/test/run-make/long-linker-command-lines-cmd-exe/Makefile b/src/test/run-make/long-linker-command-lines-cmd-exe/Makefile new file mode 100644 index 0000000000000..debe9e93824bd --- /dev/null +++ b/src/test/run-make/long-linker-command-lines-cmd-exe/Makefile @@ -0,0 +1,6 @@ +-include ../tools.mk + +all: + $(RUSTC) foo.rs -g + cp foo.bat $(TMPDIR)/ + OUT_DIR="$(TMPDIR)" RUSTC="$(RUSTC_ORIGINAL)" $(call RUN,foo) diff --git a/src/test/run-make/long-linker-command-lines-cmd-exe/foo.bat b/src/test/run-make/long-linker-command-lines-cmd-exe/foo.bat new file mode 100644 index 0000000000000..a9350f12bbb6d --- /dev/null +++ b/src/test/run-make/long-linker-command-lines-cmd-exe/foo.bat @@ -0,0 +1 @@ +%MY_LINKER% %* diff --git a/src/test/run-make/long-linker-command-lines-cmd-exe/foo.rs b/src/test/run-make/long-linker-command-lines-cmd-exe/foo.rs new file mode 100644 index 0000000000000..f9168a82e2259 --- /dev/null +++ b/src/test/run-make/long-linker-command-lines-cmd-exe/foo.rs @@ -0,0 +1,96 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Like the `long-linker-command-lines` test this test attempts to blow +// a command line limit for running the linker. Unlike that test, however, +// this test is testing `cmd.exe` specifically rather than the OS. +// +// Unfortunately `cmd.exe` has a 8192 limit which is relatively small +// in the grand scheme of things and anyone sripting rustc's linker +// is probably using a `*.bat` script and is likely to hit this limit. +// +// This test uses a `foo.bat` script as the linker which just simply +// delegates back to this program. The compiler should use a lower +// limit for arguments before passing everything via `@`, which +// means that everything should still succeed here. + +use std::env; +use std::fs::{self, File}; +use std::io::{BufWriter, Write, Read}; +use std::path::PathBuf; +use std::process::Command; + +fn main() { + if !cfg!(windows) { + return + } + + let tmpdir = PathBuf::from(env::var_os("OUT_DIR").unwrap()); + let ok = tmpdir.join("ok"); + let not_ok = tmpdir.join("not_ok"); + if env::var("YOU_ARE_A_LINKER").is_ok() { + match env::args().find(|a| a.contains("@")) { + Some(file) => { fs::copy(&file[1..], &ok).unwrap(); } + None => { File::create(¬_ok).unwrap(); } + } + return + } + + let rustc = env::var_os("RUSTC").unwrap_or("rustc".into()); + let me = env::current_exe().unwrap(); + let bat = me.parent() + .unwrap() + .join("foo.bat"); + let bat_linker = format!("linker={}", bat.display()); + for i in (1..).map(|i| i * 10) { + println!("attempt: {}", i); + + let file = tmpdir.join("bar.rs"); + let mut f = BufWriter::new(File::create(&file).unwrap()); + let mut lib_name = String::new(); + for _ in 0..i { + lib_name.push_str("foo"); + } + for j in 0..i { + writeln!(f, "#[link(name = \"{}{}\")]", lib_name, j).unwrap(); + } + writeln!(f, "extern {{}}\nfn main() {{}}").unwrap(); + f.into_inner().unwrap(); + + drop(fs::remove_file(&ok)); + drop(fs::remove_file(¬_ok)); + let status = Command::new(&rustc) + .arg(&file) + .arg("-C").arg(&bat_linker) + .arg("--out-dir").arg(&tmpdir) + .env("YOU_ARE_A_LINKER", "1") + .env("MY_LINKER", &me) + .status() + .unwrap(); + + if !status.success() { + panic!("rustc didn't succeed: {}", status); + } + + if !ok.exists() { + assert!(not_ok.exists()); + continue + } + + let mut contents = String::new(); + File::open(&ok).unwrap().read_to_string(&mut contents).unwrap(); + + for j in 0..i { + assert!(contents.contains(&format!("{}{}", lib_name, j))); + } + + break + } +}